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

[Ben Woodcroft]

Hi Vincent, thanks for the contribution.

On 26/03/16 15:49, address@hidden 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