Hello,

Please find attached v5 version of the patch. Hopefully this is the last one.

I took quite all changes from Maxim's proposal.

Things I did not took are related to html parsing. I did not use of
"%strict-tokenizer" because it needs a yet-to-be-packaged version of
Guile-Libe and did not change the result on any of my tests. I did not
take either the short-form sxpath expression for go-import meta parsing
as it is buggy on my test cases. We can revisit those choices on future
patches but for now I think I have a working version.

Other changes are mainly responses to Ludovic review.

On Tue, Mar 02, 2021 at 10:54:49PM +0100, Ludovic Courtès wrote:
> Nitpick: please mention the sections (for documentation) or variables
> changed

I tried to do it. Don't hesitate to modify message if needed before
commiting.

> (see <https://guix.gnu.org/manual/en/html_node/Submitting-Patches.html).

I finally understood that the document refers to GNU Guidelines for
Changelogs.  Some examples specific to Guix would be nice for noobs
like me.

> Some comments below, mostly stylistic as I’m not familiar with the
> actual file formats etc. that the importer implements.

I am not yet completely familiar with it either. All languages now try
to live in their own ecosystem with their own set of incompatible build
and distribution tools. I am just beginning to grasp how Go fo it.

> s/crate/go/
> s/guile-lib/Guile-Lib/

done.

> You can use ‘string-append’ instead of (string-concatenate (list …)).
> Use [[:xdigit:]] instead of [0-9A-Fa-f] for clarity and
> locale-independence.

Thanks for the string-append tip.
> 
> Also, you can arrange to use ‘make-regexp’ so that the regexp is
> compiled once for all, and then just ‘regexp-exec’:

I thought about it but was lazy. Thanks to your remark it is now done.

> Please use ‘match’ instead of car/cdr (throughout):

This one was more difficult than I thought.  It lead me to create some
specific record type, probably for the better.

> s/acons/alist-cons/ for consistency with the rest of the code.

I still must look at the difference between different type of alists. I
trusted you and just applied the substitution.

> You could rename ‘make-vcs’ above to ‘%make-vcs’ and do:
> 
>   (define (make-vcs prefix regexp type)
>     (%make-vcs prefix (make-regex regexp) type))
> 
> so that again you can rely on pre-compiled regexps.

Thanks for the tip.

I wonder when we use "%" prefix versus "*" suffix. I was under the
impression that "%" prefix was more for global (possibly mutable)
variables but you don't use it that way here.

> Keep ‘known-vcs’ in a global variable so it doesn’t have to be
> recomputed every time.

known-vcs is now a top-level variable with precompiled regexs.

> ‘guix lint’ wouldn’t like it. :-)  Maybe "Write synopsis here" instead?
> 
> Is there no info about the license?

Maxim's patch parse pkg.go.dev for synopsis, license and description.

It is not without flaws (Human review badly needed as it uses README
for trying to extract synopsis) but still better than before.

> New dependency; it’s a bit of a commitment, but hopefully Guile-Lib is
> stable enough and works on all the supported architectures.

It is a bit of commitment but we really needed a library for parsing
HTML. It is only useful on "import go" as of now so nothing critical
for using Guix itself if we can keep it optionnal.

> Please add guix/scripts/import/go.scm to ‘po/guix/POTFILES.in’ so it can
> be translated.
Done.

> > +++ b/tests/import-go.scm
> 
> Looks nice!  It should be called ‘tests/go.scm’ for consistency, with:

I renamed it. I also put in it one test for guix/build-system/go.scm.

I still am not satisfied with the overall look of this file which is
really difficult to read, but at least we have some basic tests.

> Would it be an option to also have an end-to-end test (checking the
> resulting ‘package’ sexp)?  That’d be nice, but perhaps we can add it
> afterwards if you prefer.

I added one end-to-end test loosely based on github.com/go-check/check
example.

For end-to-end tests I reused the "mock" syntax from guix/tests.scm by
doing copy-paste because use-module of "(guix tests)" was really too
slow for me. I don't know what's going on here (it seems to rebuild all
of "gnu" scheme modules) but feel free to delete the copy and import
"(guix tests)" if you prefer.

> Let’s see how much of the comments above you can address for a v4, and
> then we can get that in and improve it from there if needed!

I hope all needed to get that in the tree is done now ;-)

François