Re: [Patch] address@hidden Updated patch

[Andy Wingo]

Hi :)

Looking good!  I have some style nits :)

On Fri 03 Jun 2016 19:39, Matthew Jordan <address@hidden> writes:

> +               ;; Removing net/ tests
> +               (for-each
> +                (lambda (srcfile)
> +                  (let ((srcfile (string-append "net/" srcfile)))
> +                  (if (file-exists? srcfile)
> +                      (delete-file srcfile))))
> +                '("multicast_test.go" "parse_test.go" "port_test.go"))

Either these files exist or they do not.  Better to just have three
unconditional delete-file invocations.

> +
> +               ;; Add libgcc to runpath
> +               (substitute* "cmd/go/build.go"
> +                 (("cgoldflags := \\[\\]string\\{\\}")
> +                  (string-append "cgoldflags := []string{"
> +                                 "\"-rpath=" gcclib "\""
> +                                 "}"))
> +                 (("ldflags := buildLdflags")
> +                  (string-append
> +                   "ldflags := buildLdflags\n"
> +                   "ldflags = append(ldflags, \"-r\")\n"
> +                   "ldflags = append(ldflags, \"" gcclib "\")\n")))
> +
> +               (substitute* "os/os_test.go"
> +                 (("/usr/bin") (getcwd))
> +                 (("/bin/pwd") (which "pwd")))
> +
> +               ;; Disable failing tests
> +               (for-each
> +                (lambda (srcfile)
> +                  (substitute* (car srcfile)
> +                    (((cdr srcfile) all) (string-append all "return\n"))))

Is this a nice way to disable tests?  I dunno.  Why not change the test
name from TestFoo to DisabledTestFoo ?  Then it would not get run.

Also let's avoid car and cdr, and instead use match-lambda:

  (match-lambda
   ((file . pattern)
    ...))

But let's also avoid dotted pairs in bigger literal data structures;
it's ugly when there's lots of punctuation around.  So if you change the
lines to be like

  ("net/net_test.go" ".+TestShutdownUnix.+")

you can

  (match-lambda
   ((file pattern)
    ...))

Note the lack of " . ".

> +                (list
> +                 '("net/net_test.go" . ".+TestShutdownUnix.+")
> +                 '("net/dial_test.go" . ".+TestDialTimeout.+")

Here make the whole list a literal instead of calling `list' on a list
of quoted datums:

  '(("net/net_test.go" ".+TestHostname.+")
    ...)

Cheers,

Andy