Today
shurcooL commented on vfsgendev updates modification times in github.com/gopherjs/gopherjs5h
> 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/gopherjs5h> 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/gopherjs5h> 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 …Yesterday
shurcooL commented on Robust Header Check in github.com/bradleyfalzon/ghinstallation8h> 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/gopherjs8hThis 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/gopherjs9hYes, 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/ghinstallation10hAt 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/osext10h> 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/gopherjs1dThanks 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-humanize1dWas 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-humanize1dFor 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/gopherjs1dThanks 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/gopherjs1dAs 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/perkeep1d@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/gopherjs1dLet'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/gopherjs1dThere 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/gopherjs1dThanks 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/gopherjs1dThe 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/gopherjs1dThanks 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/gopherjs2dI'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).shurcooL commented on Move js package tests into tests pa… in github.com/gopherjs/gopherjs2dI'm going to merge this and rebase #787 on top. That way, none of the GopherJS tests will be embedded into the core packages, which has benefits (smaller compiler size, easier to modify tests).
I'm going to merge this and rebase #787 on top. That way, none of the GopherJS tests will be embedded into the core packages, which has benefits (smaller compiler size, easier to modify tests).shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs2dI often prefer Go over other tools, but a bash script seemed like a good fit for this task. It was based on the bash script in [playground repo](https://github.com/gopherjs/gopherjs.github.io/blob/b3…
I often prefer Go over other tools, but a bash script seemed like a good fit for this task. It was based on the bash script in [playground repo](https://github.com/gopherjs/gopherjs.github.io/blob/b3…shurcooL commented on Move tests in js package to tests p… in github.com/gopherjs/gopherjs2dWe already have that, except it's a build tag. It's `gopherjsdev`. If you build GopherJS with `go build -tags=gopherjsdev`, it reads from disk rather than using embedded version.
I've considered t…
We already have that, except it's a build tag. It's `gopherjsdev`. If you build GopherJS with `go build -tags=gopherjsdev`, it reads from disk rather than using embedded version.
I've considered t…shurcooL commented on Move tests in js package to tests p… in github.com/gopherjs/gopherjs2d> The issue with #800 is that the js tests are now further from the js package.
Many tests in `tests` package also test `js` and other misc GopherJS behavior. Now they're all in one place. Further…
> The issue with #800 is that the js tests are now further from the js package.
Many tests in `tests` package also test `js` and other misc GopherJS behavior. Now they're all in one place. Further…shurcooL commented on Move tests in js package to tests p… in github.com/gopherjs/gopherjs2dNot seeing any surprises with them move so far. Sent #800.
Not seeing any surprises with them move so far. Sent #800.shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs2dI can make it something like this, hope it's more clear:
```
unexpected stdout from gopherjsvendored_test.sh:
got:
unexpected output 123
want:
hello using js pkg
```
I can make it something like this, hope it's more clear:
```
unexpected stdout from gopherjsvendored_test.sh:
got:
unexpected output 123
want:
hello using js pkg
```shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs2dThat's out of scope of this PR. This `gopherjspkg` package is the same in structure as `natives`.
That's out of scope of this PR. This `gopherjspkg` package is the same in structure as `natives`.shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs2dI don't see a difference, but sure, I can take out this commit and apply it on `master` after this PR lands.
I don't see a difference, but sure, I can take out this commit and apply it on `master` after this PR lands.shurcooL commented on Move tests in js package to tests p… in github.com/gopherjs/gopherjs2d> but why are we embedding the tests in the first place?
Because a package consists of normal .go files and _test.go files. I embedded the entire package, because any other approach would be more …
> but why are we embedding the tests in the first place?
Because a package consists of normal .go files and _test.go files. I embedded the entire package, because any other approach would be more …shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs2dDid you see the commit message of 49f2b5ca9ae7415de29eb86c95d2cecaca9d606f?
However, I think I know what it'll take to make it work with `sh`. It might just be a matter of removing the `SIG` prefi…
Did you see the commit message of 49f2b5ca9ae7415de29eb86c95d2cecaca9d606f?
However, I think I know what it'll take to make it work with `sh`. It might just be a matter of removing the `SIG` prefi…shurcooL commented on client code can request the VM to r… in github.com/go-interpreter/wagon2dWhat if it's neither of those two types? Then `err` would be left `nil`.
What if it's neither of those two types? Then `err` would be left `nil`.shurcooL commented on multiple static paths in github.com/shurcooL/vfsgen2dHave you seen package [`github.com/shurcooL/httpfs/union`](https://godoc.org/github.com/shurcooL/httpfs/union)?
For example, see it being used in [here](https://github.com/shurcooL/issuesapp/blob/…
Have you seen package [`github.com/shurcooL/httpfs/union`](https://godoc.org/github.com/shurcooL/httpfs/union)?
For example, see it being used in [here](https://github.com/shurcooL/issuesapp/blob/…
shurcooL commented on Sequencing PRs to be merged in github.com/gopherjs/gopherjs3dQuick thoughts on prelude changes.
I don't quite see why we need or want #794. Shouldn't the formatting of .js files be done by a local tool? I don't think we win much by being so strict about for…
Quick thoughts on prelude changes.
I don't quite see why we need or want #794. Shouldn't the formatting of .js files be done by a local tool? I don't think we win much by being so strict about for…shurcooL commented on Add example to get a team ID from i… in github.com/google/go-github3dThere are many valid ways of doing pagination and detecting last page. This is one of them.
But I think we should be consistent with the example provided at https://godoc.org/github.com/google/go-…
There are many valid ways of doing pagination and detecting last page. This is one of them.
But I think we should be consistent with the example provided at https://godoc.org/github.com/google/go-…shurcooL commented on Support preview Team Discussions API in github.com/google/go-github3dSimilarly, what do you think of just making `comment` a value rather than pointer here as well?
Similarly, what do you think of just making `comment` a value rather than pointer here as well?shurcooL commented on Support preview Team Discussions API in github.com/google/go-github3dWe did the `if pull == nil` change for existing endpoints where a pointer was already used (see PR #539), in order to avoid making a breaking API change.
Since this is a new API, I think we can si…
We did the `if pull == nil` change for existing endpoints where a pointer was already used (see PR #539), in order to avoid making a breaking API change.
Since this is a new API, I think we can si…shurcooL commented on detect abuse rate limits on github … in github.com/google/go-github3dgooglebot's concern is not an issue; I obviously consent.
Merged as dcc59e6. Thanks @bbatha!
When you hear any updates on the ticket you submitted, please post an update here.
googlebot's concern is not an issue; I obviously consent.
Merged as dcc59e6. Thanks @bbatha!
When you hear any updates on the ticket you submitted, please post an update here.
shurcooL commented on detect abuse rate limits on github … in github.com/google/go-github3dAnd on this line too.
And on this line too.shurcooL commented on detect abuse rate limits on github … in github.com/google/go-github3dI believe Glenn was referring to the string here, which can be made "GitHub Enterprise".
I believe Glenn was referring to the string here, which can be made "GitHub Enterprise".shurcooL commented on WIP: WebcryptoAPI support in github.com/gopherjs/gopherjs3dAbout `//gopherjs:keep_overridden`, is it possible to defer that until a future PR?
A lot of code GopherJS already deals with preventing DCE by assigning to `_` (search for "DCE" in the codebase).…
About `//gopherjs:keep_overridden`, is it possible to defer that until a future PR?
A lot of code GopherJS already deals with preventing DCE by assigning to `_` (search for "DCE" in the codebase).…shurcooL commented on compiler: automate regeneration of … in github.com/gopherjs/gopherjs3dThis isn't a big deal, but it's pretty unusual to see a `[]byte` variable named `byts`. As far as I understand, it's a shortened version of "bytes"? It comes up a total of 5 times in the Go project.
…
This isn't a big deal, but it's pretty unusual to see a `[]byte` variable named `byts`. As far as I understand, it's a shortened version of "bytes"? It comes up a total of 5 times in the Go project.
…shurcooL commented on compiler: automate regeneration of … in github.com/gopherjs/gopherjs3d`bpkg.Dir` is not a very readable name here. I would've copied the `importPathToDir` helper from:
https://github.com/gopherjs/gopherjs/blob/e14987c0ef06db387b90fec85e8d06dc05598e24/compiler/native…
`bpkg.Dir` is not a very readable name here. I would've copied the `importPathToDir` helper from:
https://github.com/gopherjs/gopherjs/blob/e14987c0ef06db387b90fec85e8d06dc05598e24/compiler/native…shurcooL commented on proposal: cmpopts: add convenience … in github.com/google/go-cmp3d@dsnet What you've described sounds quite positive and has nice properties.
I just want to point out, at least with the example given, I don't see why `cmp` itself is needed to achieve that functi…
@dsnet What you've described sounds quite positive and has nice properties.
I just want to point out, at least with the example given, I don't see why `cmp` itself is needed to achieve that functi…shurcooL commented on goimports doc lists out-of-date go-… in github.com/golang/gddo3dThis issue tracker is for the gddo (godoc.org) project itself. You're referring to a package in the `x/tools` subrepo. People who work on `goimports` will not see it.
The right issue tracker is for …
This issue tracker is for the gddo (godoc.org) project itself. You're referring to a package in the `x/tools` subrepo. People who work on `goimports` will not see it.
The right issue tracker is for …shurcooL commented on x/build/cmd/gopherbot: too close-ha… in github.com/golang/go3dYet another thing I noticed recently that I suspect is related.
In issue #24850, if you look at the [issue events API](https://api.github.com/repos/golang/go/issues/24850/events), you can see goph…
Yet another thing I noticed recently that I suspect is related.
In issue #24850, if you look at the [issue events API](https://api.github.com/repos/golang/go/issues/24850/events), you can see goph…shurcooL commented on x/build/maintner: GerritMessage doe… in github.com/golang/go4dFair enough, but the tradeoff is more data.
Are you okay with the size increase from including raw `git diff-tree` output for meta commits?
Should we try to estimate or calculate how much it wo…
Fair enough, but the tradeoff is more data.
Are you okay with the size increase from including raw `git diff-tree` output for meta commits?
Should we try to estimate or calculate how much it wo…shurcooL commented on x/build/maintner: GerritMessage doe… in github.com/golang/go4dPosting some notes here after a quick investigation into where the inline comment data is.
It's available via Gerrit's git protocol (unsurprisingly). It's located inside the diff of the correspond…
Posting some notes here after a quick investigation into where the inline comment data is.
It's available via Gerrit's git protocol (unsurprisingly). It's located inside the diff of the correspond…shurcooL commented on 
Recommit feedback for @… in github.com/gopherjs/gopherjs5d@myitcv Thanks for elaborating on the tradeoffs involved. That makes sense to me. I will take that into consideration next time before making this suggestion.
> You are both, I think, wanting to g…
Recommit feedback for @… in github.com/gopherjs/gopherjs5d@myitcv Thanks for elaborating on the tradeoffs involved. That makes sense to me. I will take that into consideration next time before making this suggestion.
> You are both, I think, wanting to g…shurcooL commented on bzip2: merge into Go standard libra… in github.com/dsnet/compress5d@Darren I see your point.
I expect/hope this issue will eventually be resolved, when the conditions for it are right.
@Darren I see your point.
I expect/hope this issue will eventually be resolved, when the conditions for it are right.shurcooL commented on detect abuse rate limits on github … in github.com/google/go-github5dThanks for the ping @bbatha, we just forgot about this PR. I've requested a review from @gmlewis and we can merge once it looks good to him as well.
One more thing, have you had a chance to report…
Thanks for the ping @bbatha, we just forgot about this PR. I've requested a review from @gmlewis and we can merge once it looks good to him as well.
One more thing, have you had a chance to report…shurcooL commented on Mutation struct variable empty? in github.com/shurcooL/graphql5dThanks for following up. I'm glad you got to the bottom of this.
I think I understand what happened now. This general GraphQL client package was factored out of `graphql`, the GitHub-specific GraphQ…
Thanks for following up. I'm glad you got to the bottom of this.
I think I understand what happened now. This general GraphQL client package was factored out of `graphql`, the GitHub-specific GraphQ…shurcooL commented on Recommit feedback for @… in github.com/gopherjs/gopherjs5d I agree with @hajimehoshi, this should be `cmd.Output`.
> In the case of normal operation `CombinedOutput` == `Output`
This is not always true. Stderr can contain warnings or other information…
Recommit feedback for @… in github.com/gopherjs/gopherjs5d I agree with @hajimehoshi, this should be `cmd.Output`.
> In the case of normal operation `CombinedOutput` == `Output`
This is not always true. Stderr can contain warnings or other information…shurcooL commented on bzip2: merge into Go standard libra… in github.com/dsnet/compress6d> Surely it's more important that there is a decent implementation available in the standard library now, rather than a potential future "best" implementation?
If this package has the needed functio…
> Surely it's more important that there is a decent implementation available in the standard library now, rather than a potential future "best" implementation?
If this package has the needed functio…shurcooL commented on Return an error on Unreachable inst… in github.com/go-interpreter/wagon6dI don't have any further feedback beyond what I said above. I defer to the project owner on this.
I don't have any further feedback beyond what I said above. I defer to the project owner on this.shurcooL commented on Implement focus callback in github.com/goxjs/glfw6dThanks for the new PR. I'll able to test and review it about a week from now, sorry about the delay (busy time for me). Feel free to ping in case I forget.
Thanks for the new PR. I'll able to test and review it about a week from now, sorry about the delay (busy time for me). Feel free to ping in case I forget.shurcooL commented on Depend on Go 1.10, stop using tmp/ … in github.com/perkeep/perkeep6d@mpl There are some minor outstanding review comments I plan to address, but I won't have a chance to work on it until closer to end of the week or the weekend. So that's likely when it'll get merged…
@mpl There are some minor outstanding review comments I plan to address, but I won't have a chance to work on it until closer to end of the week or the weekend. So that's likely when it'll get merged…shurcooL commented on Support new preview Label API impro… in github.com/google/go-github6dThis has been implemented via PR #866 and merged as commit 64f8f21e24feb72595a384afcf66f9caa7106fbf.
Thanks @gjacquet!
This has been implemented via PR #866 and merged as commit 64f8f21e24feb72595a384afcf66f9caa7106fbf.
Thanks @gjacquet!shurcooL commented on compiler: support arbitrary value j… in github.com/gopherjs/gopherjs1wThanks again @myitcv!
Thanks again @myitcv!shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs1wThe failure output is:
```
$ go test -v -run=TestGopherJSCanBeVendored
=== RUN TestGopherJSCanBeVendored
--- FAIL: TestGopherJSCanBeVendored (17.78s)
gorepo_test.go:43: got != want:
got:…
The failure output is:
```
$ go test -v -run=TestGopherJSCanBeVendored
=== RUN TestGopherJSCanBeVendored
--- FAIL: TestGopherJSCanBeVendored (17.78s)
gorepo_test.go:43: got != want:
got:…shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs1wThe test is considered failed if the exit code is non-zero, or if the stdout output is not exactly the expected "hello using js pkg\n" value.
Stderr can be anything, it shouldn't fail the test. It…
The test is considered failed if the exit code is non-zero, or if the stdout output is not exactly the expected "hello using js pkg\n" value.
Stderr can be anything, it shouldn't fail the test. It…shurcooL commented on Embed core GopherJS packages into b… in github.com/gopherjs/gopherjs1wYeah. Perhaps this script can be improved in the future to infer dependencies more automatically (e.g., with `go list -f '{{join .Deps "\n"}}' github.com/gopherjs/gopherjs/...` or so).
There's a c…
Yeah. Perhaps this script can be improved in the future to infer dependencies more automatically (e.g., with `go list -f '{{join .Deps "\n"}}' github.com/gopherjs/gopherjs/...` or so).
There's a c…shurcooL commented on 
Apply minor style and d… in github.com/gopherjs/gopherjs1wIt's used in `funcContext.newVariableWithLevel`, `funcContext.translateToplevelFunction`, and `Compile`. Looks like it's for top-level identifiers or so.
I guess it's okay for it to change the nam…
Apply minor style and d… in github.com/gopherjs/gopherjs1wIt's used in `funcContext.newVariableWithLevel`, `funcContext.translateToplevelFunction`, and `Compile`. Looks like it's for top-level identifiers or so.
I guess it's okay for it to change the nam…