-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow module to be directly required #1
Conversation
Allow module to be directly required without needing a replace directive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion!
@@ -1,4 +1,4 @@ | |||
module github.com/shurcooL/graphql | |||
module github.com/cli/shurcooL-graphql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By just doing this one-line change, is the cli/cli
project that depends on this fork now immediately go-getable? Wouldn't we need to also replace every github.com/shurcooL/graphql
import directive in this fork with ithub.com/cli/shurcooL-graphql
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so, yes. I did a quick code search and beyond the go.mod
there only seem to be a few references:
- cli/cli/pkg/cmd/release/list/http.go
- cli/cli/pkg/cmd/gist/list/http.go
- cli/cli/api/client.go
That said, some of its other dependencies also refer to the same thing:
- shurcooL/githubv4/githubv4.go
- shurcooL/githubv4/scalar.go
- shurcooL/githubv4/gen.go
So perhaps such a change would not work without further forking that as well (however that module is even smaller than this one).
The political approach is to push for these:
I am not sure how long that would take though (especially since they both seem to have bogus failures in their CI).
@mislav It is too bad you cannot directly refer to shurcooL/graphql@a4a48d3 in
I tried this and |
Going to close this as #3 will address this and allow the module to be required. |
Allow module to be directly required without needing a replace directive.
This directly affects cli/cli#1389.
For reference, it might be good to consider pushing this type of thing upstream, e.g., shurcooL#53.