Today
shurcooL commented on vcs/gitcmd: Use correct commit rang… in github.com/sourcegraph/go-vcs42m
It's [failing on Go 1.5](https://travis-ci.org/sourcegraph/go-vcs/builds/371817849) because a dependency uses `math/bits`, which was added in a later version of Go.
It's [failing on Go 1.5](https://travis-ci.org/sourcegraph/go-vcs/builds/371817849) because a dependency uses `math/bits`, which was added in a later version of Go.shurcooL commented on compiler: fix variadic args not bei… in github.com/gopherjs/gopherjs7h(Actually, https://play.golang.org/p/L31k_UZViaQ is a better example of what I was pointing out.)
(Actually, https://play.golang.org/p/L31k_UZViaQ is a better example of what I was pointing out.)shurcooL commented on compiler: fix variadic args not bei… in github.com/gopherjs/gopherjs7hThanks. But why did you replace `[]string(nil)` with just `nil`? It has a [different behavior](https://play.golang.org/p/6f4GhqZc0Dn). Not critical, just pointing out.
> Please can I ask in return…
Thanks. But why did you replace `[]string(nil)` with just `nil`? It has a [different behavior](https://play.golang.org/p/6f4GhqZc0Dn). Not critical, just pointing out.
> Please can I ask in return…shurcooL commented on compiler: fix variadic args not bei… in github.com/gopherjs/gopherjs7hI want us to be more consistent with the style used in [most Go tests](https://github.com/golang/go/search?q=want), which is to print the "got" value first, the "want" value second. Mind changing it?…
I want us to be more consistent with the style used in [most Go tests](https://github.com/golang/go/search?q=want), which is to print the "got" value first, the "want" value second. Mind changing it?…shurcooL commented on OrganizationsListOptions and UserLi… in github.com/google/go-github7hThanks a lot for investigating this @sandlis!
Actually, your findings confirm to me that this issue isn't as black and white as I originally thought (where my suspected fix was to remove `ListOption…
Thanks a lot for investigating this @sandlis!
Actually, your findings confirm to me that this issue isn't as black and white as I originally thought (where my suspected fix was to remove `ListOption…shurcooL commented on compiler: fix variadic args not bei… in github.com/gopherjs/gopherjs8hWe use `t.Skip` rather than build constraints for skipping tests meant for `GOARCH=js` only, unless the build constraint is necessary for things to compile.
In this case, this test shouldn't be sk…
We use `t.Skip` rather than build constraints for skipping tests meant for `GOARCH=js` only, unless the build constraint is necessary for things to compile.
In this case, this test shouldn't be sk…shurcooL commented on client code can request the VM to r… in github.com/go-interpreter/wagon8hThe name and comment mention "execution panic", but doesn't really define it.
What is an "execution panic" and how does it differ from a normal Go run-time panic (as defined at https://golang.org/…
The name and comment mention "execution panic", but doesn't really define it.
What is an "execution panic" and how does it differ from a normal Go run-time panic (as defined at https://golang.org/…shurcooL commented on Consider support for Schema Preview… in github.com/shurcooL/githubql8hSince this library is largely generated, a viable possibility to consider is creating a separate package that includes support for the schema previews (doing this isn't as viable in go-github because…
Since this library is largely generated, a viable possibility to consider is creating a separate package that includes support for the schema previews (doing this isn't as viable in go-github because…shurcooL commented on Support preview Hovercard API. in github.com/shurcooL/githubql8hClosing in favor of a high level tracking issue #34 for all schema previews.
Closing in favor of a high level tracking issue #34 for all schema previews.shurcooL commented on Robust Header Check in github.com/bradleyfalzon/ghinstallation10hHere as well:
```
// Looks as though ...
```
You might also want to wrap the line at around 80-100 columns, so it’s more comfortable to read.
Here as well:
```
// Looks as though ...
```
You might also want to wrap the line at around 80-100 columns, so it’s more comfortable to read.shurcooL commented on Fix build for wayland in github.com/go-gl/glfw11hThanks @r0l1 and @thomasruiz for working on this, and @tapir for reviewing!
Thanks @r0l1 and @thomasruiz for working on this, and @tapir for reviewing!
shurcooL commented on Add FilesOptions to ListFiles, List… in github.com/andygrunwald/go-gerrit20hI've also bumped the minimum version tested from 1.6 to 1.8. Given that 1.10 is out now, supporting it and 2 past versions seems reasonable. Let me know if you feel strongly about supporting 1.6 and …
I've also bumped the minimum version tested from 1.6 to 1.8. Given that 1.10 is out now, supporting it and 2 past versions seems reasonable. Let me know if you feel strongly about supporting 1.6 and …shurcooL commented on Add FilesOptions to ListFiles, List… in github.com/andygrunwald/go-gerrit20h> Modify both ListFiles and ListFilesReviewed to return map/slice directly, rather than a pointer to one. There doesn't appear to be any value in returning a pointer, it just makes the API harder to …
> Modify both ListFiles and ListFilesReviewed to return map/slice directly, rather than a pointer to one. There doesn't appear to be any value in returning a pointer, it just makes the API harder to …Yesterday
shurcooL commented on Support preview Team Discussions API in github.com/google/go-github1d> But, what if user decides to pass in an empty struct. Should that be considered an error and need to be guarded against?
I think it's okay to let the GitHub API handle such an incorrect request …
> But, what if user decides to pass in an empty struct. Should that be considered an error and need to be guarded against?
I think it's okay to let the GitHub API handle such an incorrect request …shurcooL commented on Support Preview hovercard API (#87… in github.com/google/go-github1dSorry about the delay.
I see, it looks like they've updated the documentation to say that the two parameters are not mandatory, but if one is used, then the other is required too.
That means pa…
Sorry about the delay.
I see, it looks like they've updated the documentation to say that the two parameters are not mandatory, but if one is used, then the other is required too.
That means pa…shurcooL commented on OrganizationsListOptions and UserLi… in github.com/google/go-github1dI don't have permissions to send out invites, but maybe @willnorris or @gmlewis can.
In the meantime, feel free to start working on this. It should be clear to readers of this issue.
Please note, t…
I don't have permissions to send out invites, but maybe @willnorris or @gmlewis can.
In the meantime, feel free to start working on this. It should be clear to readers of this issue.
Please note, t…shurcooL commented on Robust Header Check in github.com/bradleyfalzon/ghinstallation1d> for some users not setting that header, and not using `go-github`.
Isn't this package for GitHub? How can users not be using `go-github`? Asking because I don't know.
> @shurcooL from my unde…
> for some users not setting that header, and not using `go-github`.
Isn't this package for GitHub? How can users not be using `go-github`? Asking because I don't know.
> @shurcooL from my unde…This Week
shurcooL commented on Proposal: Support avatars plugin. in github.com/andygrunwald/go-gerrit2dYes, those are good questions and we need to figure out how to answer them in order to proceed.
> How should we decide when to integrate a plugin and when not? What are the criteria?
I'm not sure m…
Yes, those are good questions and we need to figure out how to answer them in order to proceed.
> How should we decide when to integrate a plugin and when not? What are the criteria?
I'm not sure m…shurcooL commented on update.sh: fix to work with all she… in github.com/gopherjs/gopherjs.github.io2d> For me github.com/gopherjs/gopherjs/js is compiled into playground.js:
That's expected, it's there for the playground itself. It doesn't mean it would be used from there during compilation of pl…
> For me github.com/gopherjs/gopherjs/js is compiled into playground.js:
That's expected, it's there for the playground itself. It doesn't mean it would be used from there during compilation of pl…shurcooL commented on update.sh: fix to work with all she… in github.com/gopherjs/gopherjs.github.io2dThanks for providing more information.
Can you check, after you do `go generate`, are there 2 files in [./pkg/github.com/gopherjs/gopherjs](https://github.com/gopherjs/gopherjs.github.io/tree/cbde…
Thanks for providing more information.
Can you check, after you do `go generate`, are there 2 files in [./pkg/github.com/gopherjs/gopherjs](https://github.com/gopherjs/gopherjs.github.io/tree/cbde…shurcooL commented on Add PatchOptions.Path field. in github.com/andygrunwald/go-gerrit2d> For complex PRs with a couple of commits where every commit has a real use case and an explainable commit message (e.g. https://chris.beams.io/posts/git-commit/), i would disagree to do this, becau…
> For complex PRs with a couple of commits where every commit has a real use case and an explainable commit message (e.g. https://chris.beams.io/posts/git-commit/), i would disagree to do this, becau…shurcooL commented on change org id to int64 in Organizat… in github.com/google/go-github2dThanks @qinfchen!
Thanks @qinfchen!
shurcooL commented on compiler: remove dependency on lega… in github.com/gopherjs/gopherjs2dTested locally, not seeing any issues. Merging and bumping version.
Tested locally, not seeing any issues. Merging and bumping version.shurcooL commented on update.sh: fix to work with all she… in github.com/gopherjs/gopherjs.github.io2dI can't reproduce your results.
If I remove the `js.a.js` file from pkg and use `gopherjs serve`, the playground fails to run the initial program.
```
playground $ rm ./pkg/github.com/gopherjs…
I can't reproduce your results.
If I remove the `js.a.js` file from pkg and use `gopherjs serve`, the playground fails to run the initial program.
```
playground $ rm ./pkg/github.com/gopherjs…shurcooL commented on client code can request the VM to r… in github.com/go-interpreter/wagon2dOptional suggestion for how this can be phrased differently:
```Go
// RecoverExecutionPanic controls whether ExecCode method
// recovers execution panics and returns them as errors instead.
// …
Optional suggestion for how this can be phrased differently:
```Go
// RecoverExecutionPanic controls whether ExecCode method
// recovers execution panics and returns them as errors instead.
// …shurcooL commented on compiler: use hash calculation for … in github.com/gopherjs/gopherjs2d> @shurcooL, please take a look.
Sorry, but I don't have the bandwidth to properly review this change soon (maybe the coming weekend, but maybe not until the one after).
I have the following co…
> @shurcooL, please take a look.
Sorry, but I don't have the bandwidth to properly review this change soon (maybe the coming weekend, but maybe not until the one after).
I have the following co…shurcooL commented on Add PatchOptions.Path field. in github.com/andygrunwald/go-gerrit2dThanks for the quick review.
I have a handful more of local patches I'll be trying to upstream next. I'll start with the easiest and least disruptive ones first, and leave hardest ones for the end…
Thanks for the quick review.
I have a handful more of local patches I'll be trying to upstream next. I'll start with the easiest and least disruptive ones first, and leave hardest ones for the end…shurcooL commented on go generate fails when GOROOT is us… in github.com/gopherjs/gopherjs.github.io2dAlso note that this behavior is documented, see last paragraph of https://github.com/gopherjs/gopherjs#installation-and-usage.
Also note that this behavior is documented, see last paragraph of https://github.com/gopherjs/gopherjs#installation-and-usage.shurcooL commented on go generate fails when GOROOT is us… in github.com/gopherjs/gopherjs.github.io2dThat was the behavior Richard implemented about 5 years ago. His GOROOT was user writeable, and he wanted GopherJS to install its archives of the standard library into GOROOT/pkg.
Back then, it wo…
That was the behavior Richard implemented about 5 years ago. His GOROOT was user writeable, and he wanted GopherJS to install its archives of the standard library into GOROOT/pkg.
Back then, it wo…shurcooL commented on update.sh: fix to work with all she… in github.com/gopherjs/gopherjs.github.io2d> Yes, worked fine locally for me.
I see. Can you elaborate on how you tested it locally?
> Yes, worked fine locally for me.
I see. Can you elaborate on how you tested it locally?shurcooL commented on go generate fails with gopherjs 1.1… in github.com/gopherjs/gopherjs.github.io2dThat must be why then. The current `update.sh` assumes it's not. If it is, GopherJS compiler installs the `js.a` and `nosync.a` into your GOROOT instead of GOPATH, so lines 33 and 34 won't be able to…
That must be why then. The current `update.sh` assumes it's not. If it is, GopherJS compiler installs the `js.a` and `nosync.a` into your GOROOT instead of GOPATH, so lines 33 and 34 won't be able to…shurcooL commented on update.sh: fix to work with all she… in github.com/gopherjs/gopherjs.github.io2dThis change is good.
This change is good.shurcooL commented on go generate fails with gopherjs 1.1… in github.com/gopherjs/gopherjs.github.io2d> This is, I believe, because js and nosync are fully vendored.
I don't believe that is the case. Only the *source code* of those packages is embedded, not their built archives. Please see commit me…
> This is, I believe, because js and nosync are fully vendored.
I don't believe that is the case. Only the *source code* of those packages is embedded, not their built archives. Please see commit me…shurcooL commented on Robust Header Check in github.com/bradleyfalzon/ghinstallation2d> I like that idea, little confused on how I can tell if it's asking for JSON. Would it be as simple as checking the `Accept` header? If so I would think my original solution of checking all accept h…
> I like that idea, little confused on how I can tell if it's asking for JSON. Would it be as simple as checking the `Accept` header? If so I would think my original solution of checking all accept h…shurcooL commented on Where did golint go? in github.com/golang/lint2dThank you for fixing this quickly and for the explanation of how it happened.
I'm glad to hear that not just one, but two problems are resolved as a result of this, and so Go is in a better shape no…
Thank you for fixing this quickly and for the explanation of how it happened.
I'm glad to hear that not just one, but two problems are resolved as a result of this, and so Go is in a better shape no…shurcooL commented on x/tools/cmd/godoc: Does not respect… in github.com/golang/go2d> how is `go doc` different from `godoc`?
They are two different commands:
- `godoc` binary source code is at [`golang.org/x/tools/cmd/godoc`](https://golang.org/x/tools/cmd/godoc), and it typicall…
> how is `go doc` different from `godoc`?
They are two different commands:
- `godoc` binary source code is at [`golang.org/x/tools/cmd/godoc`](https://golang.org/x/tools/cmd/godoc), and it typicall…shurcooL commented on OrganizationsListOptions and UserLi… in github.com/google/go-github3dBut `RepositoryListAllOptions` does **not** have this issue:
https://github.com/google/go-github/blob/master/github/repos.go#L231-L236
But `RepositoryListAllOptions` does **not** have this issue:
https://github.com/google/go-github/blob/master/github/repos.go#L231-L236shurcooL commented on OrganizationsListOptions shouldn't … in github.com/google/go-github3dThe same issue affects `UserListOptions` struct:
https://github.com/google/go-github/blob/69ab5d997213689855e5db797eb5016ad0cc957a/github/users.go#L137-L144
Its documentation (https://developer…
The same issue affects `UserListOptions` struct:
https://github.com/google/go-github/blob/69ab5d997213689855e5db797eb5016ad0cc957a/github/users.go#L137-L144
Its documentation (https://developer…shurcooL commented on intermittent test failures / race c… in github.com/kardianos/osext3dI see, thanks for clarifying.
> Additionally, it looks like os.Executable() is only used on go 1.8, and I tested this on go 1.9.
I believe it's used on 1.8 and all newer version. What makes you…
I see, thanks for clarifying.
> Additionally, it looks like os.Executable() is only used on go 1.8, and I tested this on go 1.9.
I believe it's used on 1.8 and all newer version. What makes you…shurcooL commented on Robust Header Check in github.com/bradleyfalzon/ghinstallation3dI would suggest you consider this more general approach:
1. Check if the current request accept header asks for JSON.
2. If so, add `application/vnd.github.machine-man-preview+json` as an extra a…
I would suggest you consider this more general approach:
1. Check if the current request accept header asks for JSON.
2. If so, add `application/vnd.github.machine-man-preview+json` as an extra a…shurcooL commented on vfsgendev updates modification times in github.com/gopherjs/gopherjs3d> even if it is using modification times to determine staleness, then when reading from the VFS it will always get the same answer
Yes, it will be the same—but non-zero—answer. It would change…
> even if it is using modification times to determine staleness, then when reading from the VFS it will always get the same answer
Yes, it will be the same—but non-zero—answer. It would change…shurcooL commented on compiler: remove dependency on lega… in github.com/gopherjs/gopherjs3d> Incidentally, if there are tests/checks that you perform locally that help to give you confidence that this PR "works" we should look to add those tests/checks (or equivalent tests/checks) to the C…
> Incidentally, if there are tests/checks that you perform locally that help to give you confidence that this PR "works" we should look to add those tests/checks (or equivalent tests/checks) to the C…shurcooL commented on vfsgendev updates modification times in github.com/gopherjs/gopherjs3d> But the only thing we *need* to rely on here is the contents of those files; the modification times are irrelevant aren't they?
Is that the case? I'm not sure, but I suspect they might be used by …
> But the only thing we *need* to rely on here is the contents of those files; the modification times are irrelevant aren't they?
Is that the case? I'm not sure, but I suspect they might be used by …shurcooL commented on Robust Header Check in github.com/bradleyfalzon/ghinstallation3d> So essentially, if the request already has an Accept header, and it's equal to `application/octet-stream`, don't send the `application/vnd.github.machine-man-preview+json` header?
That's right.
…
> So essentially, if the request already has an Accept header, and it's equal to `application/octet-stream`, don't send the `application/vnd.github.machine-man-preview+json` header?
That's right.
…shurcooL commented on compiler: remove dependency on lega… in github.com/gopherjs/gopherjs3dThis line uses a very outdated style of initializing a new `bytes.Buffer`, no one writes it like that in anymore. [`bytes.NewBuffer`](https://godoc.org/bytes#NewBuffer) documentation hints at this:
…
This line uses a very outdated style of initializing a new `bytes.Buffer`, no one writes it like that in anymore. [`bytes.NewBuffer`](https://godoc.org/bytes#NewBuffer) documentation hints at this:
…shurcooL commented on vfsgendev updates modification times in github.com/gopherjs/gopherjs3dYes, this is a known limitation of `vfsgen`, and the tracking issue for it is https://github.com/shurcooL/vfsgen/issues/26.
> Am I right in thinking this is because the modification times are taken …
Yes, this is a known limitation of `vfsgen`, and the tracking issue for it is https://github.com/shurcooL/vfsgen/issues/26.
> Am I right in thinking this is because the modification times are taken …shurcooL commented on Robust Header Check in github.com/bradleyfalzon/ghinstallation4dAt a high level, this makes sense. I think the implementation can hopefully be made simpler.
@bradleyfalzon See a relevant issue https://github.com/google/go-github/issues/870 for more context.
At a high level, this makes sense. I think the implementation can hopefully be made simpler.
@bradleyfalzon See a relevant issue https://github.com/google/go-github/issues/870 for more context.shurcooL commented on intermittent test failures / race c… in github.com/kardianos/osext4d> I just checked again, and this issue is still present.
But the README says:
> As of go1.8 the Executable function may be found in `os`. The Executable function
in the std lib `os` package is…
> I just checked again, and this issue is still present.
But the README says:
> As of go1.8 the Executable function may be found in `os`. The Executable function
in the std lib `os` package is…
shurcooL commented on compiler: fix handling of struct, a… in github.com/gopherjs/gopherjs4dThanks for working on this bug, and sorry it's taking so long. I've found some time and started reviewing this now. I will need more time, but I found something that I haven't seen mentioned so far. …
Thanks for working on this bug, and sorry it's taking so long. I've found some time and started reviewing this now. I will need more time, but I found something that I haven't seen mentioned so far. …Last Week
shurcooL commented on Commaf which rounds to X decimal pl… in github.com/dustin/go-humanize5dWas this issue resolved by PR #60 (which has been recently merged), or is there more to do here? /cc @bramp
Was this issue resolved by PR #60 (which has been recently merged), or is there more to do here? /cc @brampshurcooL commented on Add a few funcs for limiting the nu… in github.com/dustin/go-humanize5dFor posterity (since it's not very visible), this was merged via commit 41f808901d8eb7d8d4e70a79b62b0d1b504e3f9a.
For posterity (since it's not very visible), this was merged via commit 41f808901d8eb7d8d4e70a79b62b0d1b504e3f9a.shurcooL commented on X-Content-Type-Options: nosniff wil… in github.com/gopherjs/gopherjs5dThanks for helping diagnose the issue @myitcv. It sounds like it's resolved now, the problem was that the HTML included a script with the wrong URL, which gave 404.
I'll close this because it's no…
Thanks for helping diagnose the issue @myitcv. It sounds like it's resolved now, the problem was that the HTML included a script with the wrong URL, which gave 404.
I'll close this because it's no…shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs5dAs a followup step, I've checked that the GopherJS Playground works with the latest version (it does) and updated it in https://github.com/gopherjs/gopherjs.github.io/commit/cbdec896dfc8a335278926fc7…
As a followup step, I've checked that the GopherJS Playground works with the latest version (it does) and updated it in https://github.com/gopherjs/gopherjs.github.io/commit/cbdec896dfc8a335278926fc7…shurcooL commented on Depend on Go 1.10, stop using tmp/ … in github.com/perkeep/perkeep5d@mpl That PR (https://github.com/gopherjs/gopherjs/pull/787) has been merged, so you can switch to using GopherJS 1.10-3 (with support for being vendored) from `master` branch now.
@mpl That PR (https://github.com/gopherjs/gopherjs/pull/787) has been merged, so you can switch to using GopherJS 1.10-3 (with support for being vendored) from `master` branch now.shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs5dLet's discuss that outside this PR. I've already effectively allocated 1.10-3 for this fix by bumping to 1.10-3 in the earlier version of this PR, so that release has already been decided.
We can …
Let's discuss that outside this PR. I've already effectively allocated 1.10-3 for this fix by bumping to 1.10-3 in the earlier version of this PR, so that release has already been decided.
We can …shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs5dThere isn't a need, that PR is orthogonal. We can bump up to 1.10-4 for it if needed.
There isn't a need, that PR is orthogonal. We can bump up to 1.10-4 for it if needed.shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs5dThanks for the reviews!
I'm going to merge this and bump up the GopherJS compiler version to 1.10-3, the first version to support vendoring GopherJS into other Go projects.
Thanks for the reviews!
I'm going to merge this and bump up the GopherJS compiler version to 1.10-3, the first version to support vendoring GopherJS into other Go projects.shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs5dThe motivation is written in the commit message of d0d69c08b6af458cebb60d3295f643e4a471029d:
> Rely on runtime.GOARCH and t.Skip to skip tests that shouldn't be run
> with GopherJS. It's more cle…
The motivation is written in the commit message of d0d69c08b6af458cebb60d3295f643e4a471029d:
> Rely on runtime.GOARCH and t.Skip to skip tests that shouldn't be run
> with GopherJS. It's more cle…shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs5dThanks for the suggestion. I wasn't familiar with this syntax, but I'll consider it next time.
Thanks for the suggestion. I wasn't familiar with this syntax, but I'll consider it next time.shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs5dI've rebased this PR on top of latest `master` and addressed all outstanding comments. PTAL.
(Previous PR version was 2ad4a5b9a30e5f1f31c643c2a2ac27543a5ddb58).
I've rebased this PR on top of latest `master` and addressed all outstanding comments. PTAL.
(Previous PR version was 2ad4a5b9a30e5f1f31c643c2a2ac27543a5ddb58).