Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Relax remote comparison #41

Closed
wants to merge 2 commits into from
Closed

Relax remote comparison #41

wants to merge 2 commits into from

Conversation

FiloSottile
Copy link

No description provided.

@dmitshur
Copy link
Member

dmitshur commented Mar 19, 2017

Hi Filippo, thanks for contributing!

Can I ask what's the problem you're trying to resolve with this PR? What's the rationale for applying this change?

I'd also like to point your attention towards the following test case in ./status package, which is currently failing in this PR:

{
	// .git suffix does not have special treatment, and these are not considered as equal URLs.
	// See https://github.com/shurcooL/gostatus/issues/37#issuecomment-225336823 for rationale.
	rawurl0: "https://github.com/user/repo",
	rawurl1: "https://github.com/user/repo.git",
	want:    false,
},

Source:

{
// .git suffix does not have special treatment, and these are not considered as equal URLs.
// See https://github.com/shurcooL/gostatus/issues/37#issuecomment-225336823 for rationale.
rawurl0: "https://github.com/user/repo",
rawurl1: "https://github.com/user/repo.git",
want: false,
},
.

The behavior you're changing in this PR is not an accidental bug, but rather a conscious design decision. As the comment points out, please see #37 (comment) for full rationale, if you haven't seen it already. What are your thoughts on that?

@dmitshur
Copy link
Member

dmitshur commented Apr 15, 2017

@FiloSottile, did you see my comment above?

@FiloSottile
Copy link
Author

Hi Dmitri, oops, I didn't notice the previous discussion.

The motive behind the PR is simple. I took the tool, pointed it at an existing, messy GOPATH, with the task of cleaning it up, which is exactly what's it has been great for. There were some cases that required manual intervention that the tool could have safely handled automatically, so I added support for them.

I also see the simplicity argument. Were it me, I'd take the convenience tradeoff. But it's your project, and I won't be offended by the slightest if you decide to close the PR. Otherwise, let me know and I'll fix the tests.

Sorry for not doing my homework, and thanks for the tool!

@dmitshur
Copy link
Member

dmitshur commented Apr 20, 2017

Thanks for the explanation @FiloSottile.

I took the tool, pointed it at an existing, messy GOPATH, with the task of cleaning it up, which is exactly what's it has been great for.

That's exactly what I created it for. 👍 Glad it's helpful.

There were some cases that required manual intervention that the tool could have safely handled automatically, so I added support for them.

I also see the simplicity argument. Were it me, I'd take the convenience tradeoff. But it's your project, and I won't be offended by the slightest if you decide to close the PR. Otherwise, let me know and I'll fix the tests.

Understandable.

I actually have some additional thoughts on what can be done to improve this situation.

This issue is currently in a "NeedsDecision" status. Once a decision is made, the implementation is trivial and I can do it myself. I'm already tracking the decision and having discussion at shurcooL/Go-Package-Store#78, so I'll close this PR to centralize the discussion. I suggest you follow that issue if you're interested in updates.

I've also reported an issue with go get -u behavior upstream at golang/go#20039. Since gostatus is meant to follow and augment cmd/go, it's likely I'll end up adopting whatever decision gets made upstream.

Just as a heads up, in the meantime, you can still use -f flag to disable remote URL checking.

  -f	Force not to verify that each package has been checked out from the source
control repository implied by its import path. This can be useful if the source is
a local fork of the original.

Sorry for not doing my homework, and thanks for the tool!

No problem. Thanks for using it, and for trying to contribute!

@dmitshur dmitshur closed this Apr 20, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants