Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
improve redirection to canonical import path #560
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
|
I signed it! |
|
CLAs look good, thanks! |
|
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Dmitri Shuralyov: Patch Set 2: Code-Review-1 Thanks for working on this change. I think we can and should solve the bug (#507) without introducing this "" notation. The import path comment already serves that purpose, and it would be confusing and suboptimal if there's a second way. I suggest we discuss a design for how to solve the bug in issue #507 first. Once there's agreement on a good solution, proceeding to implementing it makes more sense. Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Martin Tournoij: Patch Set 2:
Yes, I agree; it's just that as how I understand the code, it's very hard to do it in another way. But could be mistaken there, of course. Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Martin Tournoij: Patch Set 3:
Did you have a chance to look at the revised patch Dmitri? I think that should fix it, and it seems to work well in all cases I can think of. Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Dmitri Shuralyov: Patch Set 3:
Thanks for the ping Martin. Unfortunately, I'm in the middle of a move, so things are a bit hectic and it's hard for me to find a continuous time block to focus on this. I hope to get to this within a week, but please ping again if I don't. One thing I can see right away is that the Commit Message is in need of an update to describe the new implementation. Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
Problem: Our GitHub organisation is named Teamwork, but for various hard to change reasons we import packages as teamwork (lower case t). To enforce this we add an import enforcement for our packages: package duck // import "github.com/teamwork/duck" This works well, but godoc.org enforces users to use the canonical GitHub path (Teamwork), but with the import statement the Go compiler enforces a different path, leading to an error. Fix: Instead of immediately redirecting in the gosrc package, it will now only store the canonical import path there. This is before the Go source code is actually scanned, so we don't *actually* know what the real canonical path should be. In the doc.newPackage() function it will check for the canonical import path, taking both the Go import path as well as the reported path from the gosrc package in to account, and redirect as needed. This seems to work for all the cases I can think of: github.com/Teamwork/validate -> github.com/teamwork/validate github.com/pkg/ERRORS -> github.com/pkg/errors github.com/Carpetsmoker/sconfig -> arp242.net/sconfig arp242.net/SCONFIG -> not found (expected) bitbucket.org/pkg/inflect -> works bitbucket.org/pkg/INFLECT -> works (should probably redirect too) github.com/docker/docker/client -> works (#534) github.com/moby/moby/client -> redirects (on master this actually seems to error out?) It should also be easy to add a similar check for for some other repo providers, such as BitBucket, GitLab, etc. Fixes #507
|
Message from Gerrit User 24958: Patch Set 4:
Sure, take your time. No hurries! It's just that sometimes it's hard to see if someone is just very busy, or has forgotten about a patch/PR :-)
I updated it in GitHub (git rebase -i origin/master), but it doesn't seem to show here. It does say "Uploaded patch set 4" though, hmmm. Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 6005: Patch Set 4:
Yep, that's why it's completely okay to send a ping after a week of inactivity.
Take a look at https://github.com/golang/go/wiki/GerritBot#how-does-gerritbot-determine-the-final-commit-message:
Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 12446: Uploaded patch set 5: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 12446: Uploaded patch set 6: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 6005: Patch Set 6: (5 comments) Thank you for updating the commit message, and for your patience, it's greatly appreciated! I'm sorry about the excessive delay with the review. I've reviewed this change with Tuo (/cc'ed), and we think the approach at a high level is sound. There are some implementation details we'd like to see changed. I left inline review comments. Once applied, I expect this should be very close to being something we can test and merge. I should be able to iterate quicker on reviews now. In addition to the inline comments, can you take a look and see if if it's viable to update/add some tests to ensure the new behavior doesn't regress? Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 24958: Patch Set 6: Thanks!
I'm not sure if that's an easy thing to do:
It's of course perfectly possible to add tests with some refactoring and writing of helper functions, but I'm not sure if that's in scope for this (relatively minor) patch? It is probably better to discus an approach for testing in another issue/patch? Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 12855: Patch Set 7: Code-Review+1 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 24958: Patch Set 7: (6 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 6005: Patch Set 7: (1 comment)
Fair enough. Thanks for looking. Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 24958: Patch Set 8: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 6005: Patch Set 8: Code-Review+2 (2 comments) I tested the new version, it seems to handle the problematic case well, as well as all previous problems that I could find. LGTM. Thank you. See minor inline comments. One of the things we should be careful about is introducing regressions in other areas. Without tests, making changes is riskier. If we run into issues in the futures, it'll make sense to start investing into tests as part of the solution. Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 6005: Patch Set 9: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 12855: Patch Set 9: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
|
Message from Gerrit User 12446: Uploaded patch set 10: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. |
Problem:
Our GitHub organisation is named Teamwork, but for various hard to
change reasons we import packages as teamwork (lower case t). To enforce
this we add an import enforcement for our packages:
package duck // import "github.com/teamwork/duck"
This works well, but godoc.org enforces users to use the canonical
GitHub path (Teamwork), but with the import statement the Go compiler
enforces a different path, leading to an error.
Fix:
Instead of immediately redirecting in the gosrc package, it will now
only store the canonical import path there. This is before the Go source
code is actually scanned, so we don't *actually* know what the real
canonical path should be.
In the doc.newPackage() function it will check for the canonical import
path, taking both the Go import path as well as the reported path from
the gosrc package in to account, and redirect as needed.
This seems to work for all the cases I can think of:
github.com/Teamwork/validate -> github.com/teamwork/validate
github.com/pkg/ERRORS -> github.com/pkg/errors
github.com/Carpetsmoker/sconfig -> arp242.net/sconfig
arp242.net/SCONFIG -> not found (expected)
bitbucket.org/pkg/inflect -> works
bitbucket.org/pkg/INFLECT -> works (should probably redirect too)
github.com/docker/docker/client -> works (#534)
github.com/moby/moby/client -> redirects (on master this actually seems to error out?)
It should also be easy to add a similar check for for some other repo
providers, such as BitBucket, GitLab, etc.
Fixes #507
Change-Id: I49c5ccb328694f95dd7a0e5d577297d56d88893f
GitHub-Last-Rev: 28efc37
GitHub-Pull-Request: #560
Reviewed-on: https://go-review.googlesource.com/120059
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
|
This PR is being closed because golang.org/cl/120059 has been merged. |
Problem:
Our GitHub organisation is named Teamwork, but for various hard to
change reasons we import packages as teamwork (lower case t). To enforce
this we add an import enforcement for our packages:
This works well, but godoc.org enforces users to use the canonical
GitHub path (Teamwork), but with the import statement the Go compiler
enforces a different path, leading to an error.
Fix:
Instead of immediately redirecting in the gosrc package, it will now
only store the canonical import path there. This is before the Go source
code is actually scanned, so we don't actually know what the real
canonical path should be.
In the doc.newPackage() function it will check for the canonical import
path, taking both the Go import path as well as the reported path from
the gosrc package in to account, and redirect as needed.
This seems to work for all the cases I can think of:
It should also be easy to add a similar check for for some other repo
providers, such as BitBucket, GitLab, etc.
Fixes #507