From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Woodcroft Subject: Re: On my way to my first patch, need review Date: Sun, 27 Mar 2016 22:11:54 -0700 Message-ID: <56F8BD1A.1000400@uq.edu.au> References: Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="------------050505020208000404030403" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:56965) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akPTJ-0003pT-Ts for guix-devel@gnu.org; Mon, 28 Mar 2016 01:12:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1akPTG-0005eE-L6 for guix-devel@gnu.org; Mon, 28 Mar 2016 01:12:13 -0400 Received: from mailhub2.soe.uq.edu.au ([130.102.132.209]:36880 helo=newmailhub.uq.edu.au) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akPTG-0005Zs-2j for guix-devel@gnu.org; Mon, 28 Mar 2016 01:12:10 -0400 In-Reply-To: List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: vincent@cloutier.co, Guix Devel This is a multi-part message in MIME format. --------------050505020208000404030403 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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 --------------050505020208000404030403 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-MIME-Autoconverted: from 8bit to quoted-printable by newmailhub.uq.edu.au id u2S5BwSM015047 Hi Vincent, thanks for the contribution.

On 26/03/16 15:49, vincent@cloutier.c= o 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
=C2=A0 (package
=C2=A0=C2=A0=C2=A0 (name "wayback-machine-downloader")
=C2=A0=C2=A0=C2=A0 (version "0.2.1")
=C2=A0=C2=A0=C2=A0 (source
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (origin
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (method url-fetch)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (uri (rubygems-uri
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 "wayback_machine_downloader"
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 version))

Please put the above 3 lines on the same line.

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

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
=C2=A0 (package
=C2=A0=C2=A0=C2=A0 (name "ruby-nokogumbo")
=C2=A0=C2=A0=C2=A0 (version "1.4.6")
=C2=A0=C2=A0=C2=A0 (source (origin
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 ;; We use the git reference, because there's no Rakefile in the
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 ;; published gem and the tarball on Github is outdated.
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 (method git-fetch)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 (uri (git-reference
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (url "https://git= hub.com/rubys/nokogumbo.git")
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (commit "d56f954d20a"))) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 (file-name (string-append name "-" version "-checkout"))
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 (sha256
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 (base32
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 "0bnppjy96xiadrsrc9dp8y6wvdwnkfa930n7acrp0mqm4qywl2wl"))))


=C2=A0=C2=A0=C2=A0 (native-inputs
=C2=A0=C2=A0=C2=A0=C2=A0 `(("ruby-rake-compiler" ,ruby-rake-compile= r)

Is this really needed?

=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ("ruby-minitest" ,ruby-minitest)))

=C2=A0=C2=A0=C2=A0 (synopsis
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "Download website from archive.org's= Wayback Machine")

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

=C2=A0= =C2=A0=C2=A0 (description
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "Download any website from the Wayba= ck Machine.=C2=A0 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.=C2=A0 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.

=C2=A0= =C2=A0=C2=A0 (home-page
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "https://github.com/hartator/wayback-machine-dow= nloader")
=C2=A0=C2=A0=C2=A0 (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`,=C2=A0 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
--------------050505020208000404030403--