Thanks for your review, the patch number 2 based on 1 should fix all the
problems you noted.

Leo Famulari <leo@famulari.name> writes:

> On Sat, Feb 06, 2016 at 07:45:53PM +0100, Nils Gillmann wrote:
>
> [...]
>
>>     * gnu/packages/lisp.scm (lispf4): New variable.
>
> [...]
>
>> +(define-public lispf4
>> +  (let* ((commit "174d8764d2f9764e8f4794c2e3feada9f9c1f1ba"))
>
> I believe you can use "let" instead of "let*".
>
true. fixed.
>> +         (package
>             ^
> Can you alter the indentation so that this ( is underneath the e in let?
> This will help with line length issues. (I usually refer to the package
> definition for fxtract when fetching from git repos.)
>
> [...]
>
>> +             #:phases
>> +             (modify-phases %standard-phases
>> +               (delete 'configure)
>> +               (replace
>> +                'install
>> +                (lambda* (#:key outputs inputs #:allow-other-keys)
>> +                  (let* ((out (assoc-ref outputs "out"))
>> +                         (bin-dir (string-append out "/bin")))
>> +                    ;; Make directories
>> +                    (mkdir-p bin-dir)
>> +                    ;; copy files
>
> We have a function install-file (guix/build/utils.scm) that combines
> mkdir-p and copy-file. It reduces the boilerplate a bit. Could you use
> that? Also we probably don't need comments such as "copy file".
removed most comments, used install-file and copy-recursively.
>
>> +                    (copy-file "lispf4"
>> +                               (string-append bin-dir "/lispf4"))
>> +                    (copy-file "SYSATOMS"
>> +                               (string-append bin-dir "/SYSATOMS"))
>> +                    (let* ((doc (assoc-ref outputs "doc"))
>> +                           (doc-dir (string-append doc "/share/doc/lispf4")))
>> +                      ;; Make directory
>> +                      (mkdir-p doc-dir)
>> +                      (copy-file "Documentation/DevelopmentProcess.txt"
>> +                                 (string-append doc-dir "/DevelopmentProcess.txt"))
>> +                      (copy-file "Documentation/Haraldson-LISP_details.pdf"
>> +                                 (string-append doc-dir "/Haraldson-LISP_details.pdf"))
>
> For some reason the linter doesn't complain about this line (and others)
> being too long. I guess it measures from the beginning of 'package'
> rather than the first column. Can you make sure they are all ≤ 80
> characters after changing the indentation as requested above?
okay, I did not notice the intendation was off. I need to find some
visual help for that.
>
>> +                      (copy-file "Documentation/ImplementationGuide.txt"
>> +                                 (string-append doc-dir "/ImplementationGuide.txt"))
>> +                      (copy-file "Documentation/Interlisp-Oct_1978.pdf"
>> +                                 (string-append doc-dir "/Interlisp-Oct_1978.pdf"))
>> +                      (copy-file "Documentation/p501-schorr.pdf"
>> +                                 (string-append doc-dir "/p501-schorr.pdf"))
>> +                      (copy-file "Documentation/README.txt"
>> +                                 (string-append doc-dir "/README.txt"))
>> +                      (copy-file "Documentation/UsersGuide.txt"
>> +                                 (string-append doc-dir "/UsersGuide.txt")))
>> +                    #t))))))
>> +          (synopsis "InterLisp interpreter")
>> +          (description
>> +           "LISPF4 is an InterLisp interpreter written in FORTRAN by Mats Nordstrom
>> + (Uppsala, Sweden) in the early 80's.  It was converted to C by Blake McBride.
>      ^
>      We probably don't need to provide this level of historical
>      detail.
noted, and removed most unnecessary details.
>
>> +It supports much of the InterLisp Standard.  Interlisp is a dynamically
>> +scoped lisp system.  It supports LAMBDA (evaluates function arguments),
>> +NLAMBDA (doesn't evaluate its arguments), and variable number of arguments.
>> +  Macros are supported as well.  The original user manual, implementors manual
>> +and the documentation can be obtained through 'guix -i lispf4:doc'.")
>
> I don't think it's necessary to provide usage tips in the description.
>
>> +          (home-page "https://github.com/blakemcbride/LISPF4.git")
>> +          (license license:expat))))
>> -- 
>> 2.6.3
>> 
>> (is there a prefered way btw? .patch file attached or just file inserted/pasted?)
>
> When using `git send-email`, the patches are automatically sent in their
> own messages, which makes it trivial to apply them with `git am`.  Any
> other method introduces some friction.
>
> I typically do all my work on branches, so this works for me:
> $ git send-email --to guix-devel@gnu.org --cover-letter --annotate -n --thread=shallow master

I am still new to more regular patch creating with git,
so the 2nd patch has no annotations of what changed,
but it's technically still the first commit. This will
change in the near future, but currently I can only do it this way.