Re: On my way to my first patch, need review

[Ben Woodcroft <b.woodcroft@uq.edu.au>]

[-- Attachment #1: Type: text/plain, Size: 4526 bytes --]

Hi Vincent, thanks for the contribution.

On 26/03/16 15:49, vincent@cloutier.co wrote:
> [..]

First some comments on the patch. In general we try to keep line lengths 
below 80 chars, and not leave blank lines within package definitions.

>
> (define-public wayback-machine-downloader
>   (package
>     (name "wayback-machine-downloader")
>     (version "0.2.1")
>     (source
>       (origin
>         (method url-fetch)
>         (uri (rubygems-uri
>                "wayback_machine_downloader"
>                version))

Please put the above 3 lines on the same line.

> (sha256
>           (base32
> "1nrwm5bc7vqm02m2x0lylxyya446kg0spg6ksi7dfkrad0l9jq8y"))))
>     (build-system ruby-build-system)
>     (arguments
>        `(#:tests? #f ; no rakefile
>               ))

Stylistically, we don't leave empty braces alone like this.

It seems there is a Rakefile with a test task defined in the github 
repository, but not in the gem itself. So in this case I would point 
guix to download the source from the github page not rubygems.org, as is 
currently done for ruby-nokogumbo for example:

(define-public ruby-nokogumbo
   (package
     (name "ruby-nokogumbo")
     (version "1.4.6")
     (source (origin
               ;; We use the git reference, because there's no Rakefile 
in the
               ;; published gem and the tarball on Github is outdated.
               (method git-fetch)
               (uri (git-reference
                     (url "https://github.com/rubys/nokogumbo.git")
                     (commit "d56f954d20a")))
               (file-name (string-append name "-" version "-checkout"))
               (sha256
                (base32
"0bnppjy96xiadrsrc9dp8y6wvdwnkfa930n7acrp0mqm4qywl2wl"))))

>
>     (native-inputs
>      `(("ruby-rake-compiler" ,ruby-rake-compiler)

Is this really needed?

> ("ruby-minitest" ,ruby-minitest)))
>
>     (synopsis
>       "Download website from archive.org's Wayback Machine")

Maybe "Download a website ..." ? Also, please put on one line.

> (description
>       "Download any website from the Wayback Machine.  Wayback Machine 
> by Internet Archive (archive.org) is an awesome tool to view any 
> website at any point of time but lacks an export feature.  Wayback 
> Machine Downloader brings exactly this.")

I would remove "awesome" as being too opinionated. I think it would also 
make sense to comment on the fact that it is a command line tool.

> (home-page
>       "https://github.com/hartator/wayback-machine-downloader")
>     (license expat)))
>
>
> Then I ran: `guix package -i wayback-machine-downloader -f 
> ~/Documents/ruby` and it successfully installed.
>
> So my questions are:
>
> 1) Do you guys and gals have a better workflow that includes the git 
> repo, so I can send a patch? All I saw in the documentation was about 
> building guix itself. I guess I could clone somewhere and use `guix 
> package -f`,  but will this be a reliable way of testing? And will 
> this make my guix less stable on the long run?

My personal workflow for this situation would be something like this:

$ git checkout -b wayback-machine-downloader origin/master
$ make -j4
... modify gnu/packages/ruby.scm adding the package definition ...
$ ./pre-inst-env guix build -K wayback-machine-downloader
... modify further as necessary until I am happy ...
... run through the checklist of things to do (e.g. list) when 
submitting a package at 
https://www.gnu.org/software/guix/manual/guix.html#Submitting-Patches
$ git diff #check that I'm committing the right code and only the one 
package's code
$ git commit
... edit commit message ...
$ git format-patch -1
... send to guix-devel ...

hope that helps.

>
> 2) Should I add "ruby-" before the name of the package? I know 
> technically all gems should have "ruby-" before the name, but this is 
> designed to be use independently. Could it have multiple names, or is 
> it a bad idea?

In this case I would say no, leave it as wayback-machine-downloader 
since it is not intended to be used as a library, rather a command line 
tool.

>
> 3) Where does this package belong in the directory?

My guess would be web.scm not ruby.scm.

>
> 4) Is the package declaration itself all right? Are packages sorted or 
> organized in any way?

This depends on the file being modified, in ruby.scm we just add it to 
the end of the file.

>
> 5) I speak fluent French, can I add a description and summary in French?

I'm not sure on this sorry.

Would you mind submitting an updated patch please?
Thanks,
ben

[-- Attachment #2: Type: text/html, Size: 7722 bytes --]