On Wed, 12 Jun 2019 21:48:52 +0200
Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:

> Hello,
> 
> Jesse Gibbons <jgibbons2357@gmail.com> writes:
> 
> > Patch is attached. -Jesse  
> 
> Thank you! Some comments follow.
> 
> > From cca52f508e84ac34e60a3c5167554d7ad2ad6564 Mon Sep 17 00:00:00
> > 2001 From: Jesse Gibbons <jgibbons2357+guix@gmail.com>
> > Date: Wed, 12 Jun 2019 10:07:32 -0600
> > Subject: [PATCH] add freeorion  
> 
> The commit message is incomplete. It should also include
> 
>     * gnu/packages/games.scm (freeorion): New variable.
> 
> If you use Emacs, there is a template to automatically fill this.
I did not know this. How do I use the template?
>
> > +     (origin
> > +       (method git-fetch)
> > +       (uri (git-reference
> > +             (url "https://github.com/freeorion/freeorion.git")
> > +             (commit
> > "470d0711537804df3c2ca25532f674ab4bec58af")))  
> 
> Why do you need to use the latest commit instead of the latest stable
> release? Unless there is a good reason, Guix prefers using stable
> releases:
The most recent release won't build because it expects a dependency
that no longer exists. I do not know which of the thousands of commits
since then fixed that issue so, I cannot easily generate a patch. It
follows that the best choice is to use the most recent commit, which I
can confirm does not have that critical issue.

I added a comment to explain why I specify the most recent commit. I
also added a note that it should be updated when the next stable
release is available (the maintainers seem to prefer announcing new
releases every September). I will personally update this in
mid-September or October if version 0.4.8.1 or 0.4.9 or 0.5 or 1.0 is
available and no volunteer beats me to it. If you want I can also
request release 0.4.8.1 ASAP so we can specify a release rather than a
commit. I personally prefer to keep the commit and wait three or four
months for the release.
> 
>     (commit version)
> 
> > +       (sha256
> > +        (base32
> > +         "1wsw632l1cj17px6i88nqjzs0dngp5rsr67n6qkkjlfjfxi69j0f"))))
> > +    (arguments
> > +     '(#:tests? #f))  
> 
> You should include a comment explaining why tests are removed. It
> could be, for example,
> 
>     '(#:tests? #f))   ;no test 
> 
> > +    (home-page
> > +     "https://www.freeorion.org/index.php/Main_Page")  
> 
> I think "https://www.freeorion.org" is enough, since it points to the
> page above.
> 
> > +    (description
> > +     "FreeOrion is a free, open source, turn-based space empire
> > and galactic +conquest (4X) computer game being designed and built
> > by the FreeOrion project. +FreeOrion is inspired by the tradition
> > of the Master of Orion games, but is not +a clone or remake of that
> > series or any other game.")  
> 
> It may be useful to explain what the "4X" means, or do not include it
> at all. Note that in the wiki, "4X" is a link, so you can get further
> information.
The link says that 4X refers to a genre of strategy game centered
around conquest. I think the current context is a sufficient
explanation, but I did expand the description. I think it is best to
keep the 4X in the description in case someone tries a search for that
genre.
> 
> > +    (license (list license:gpl2 license:cc-by-sa3.0))))  
> 
> Could you explain, in a comment, why there are two licenses? I assume
> the former is the project, and the latter is for assets.
I added comments to explain these licenses.
> 
> Would you mind sending an updated patch?
> 
> Regards,
> 
Updated patch is attached. Is there anything else you want me to fix?