search

golang / gddo

Go Doc Dot Org
https://godoc.org
BSD 3-Clause "New" or "Revised" License
1.1k stars 266 forks source link

improve redirection to canonical import path #560

Closed arp242 closed 6 years ago

arp242 commented 6 years ago

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

googlebot commented 6 years ago

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).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers
arp242 commented 6 years ago

I signed it!

googlebot commented 6 years ago

CLAs look good, thanks!

gopherbot commented 6 years ago

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps: Within the next week or so, a maintainer will review your change and provide feedback. See https://golang.org/doc/contribute.html#review for more info and tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be surprising to people new to the project. The careful, iterative review process is our way of helping mentor contributors and ensuring that their contributions have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which little code gets reviewed or merged. If a reviewer responds with a comment like R=go1.11, it means that this CL will be reviewed as part of the next development cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

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 (https://github.com/golang/gddo/issues/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 golang/gddo#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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Martin Tournoij:

Patch Set 2:

Patch Set 2: Code-Review-1 I think we can and should solve the bug (https://github.com/golang/gddo/issues/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.

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Martin Tournoij:

Patch Set 3:

Patch Set 2: Code-Review-1

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Dmitri Shuralyov:

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.

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Gerrit User 24958:

Patch Set 4:

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.

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 :-)

One thing I can see right away is that the Commit Message is in need of an update to describe the new implementation.

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Gerrit User 6005:

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 :-)

Yep, that's why it's completely okay to send a ping after a week of inactivity.

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.

Take a look at https://github.com/golang/go/wiki/GerritBot#how-does-gerritbot-determine-the-final-commit-message:

How does GerritBot determine the final commit message?

It uses the title and description of the PR to construct the commit message for the Gerrit Change.

Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Gerrit User 24958:

Patch Set 6:

Thanks!

can you take a look and see if if it's viable to update/add some tests

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Gerrit User 24958:

Patch Set 7:

(6 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Gerrit User 6005:

Patch Set 7:

(1 comment)

I'm not sure if that's an easy thing to do:

• doc.newPackage() parses the Go code, so we need to set up a GOPATH somewhere. • gosrc.getGitHubDir() calls the GitHub API.

Fair enough. Thanks for looking.

Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Gerrit User 24958:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Gerrit User 6005:

Patch Set 9:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

Message from Gerrit User 12855:

Patch Set 9:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/120059. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

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. After addressing review feedback, remember to publish your drafts!

gopherbot commented 6 years ago

This PR is being closed because golang.org/cl/120059 has been merged.