-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Module aware generator #1436
Module aware generator #1436
Conversation
Cobra and Viper are great together, but it's not uncommon to use them apart. New Cobra users don't know better and including Viper by default adds complexity to the skeleton.
It's questionable that a default license makes any sense from a legal perspective. If the tool created the license without the user choosing it, then it may not even be applicable. Best to let the user choose their license with intent.
Pretty major change in behavior, but with modules a change is needed. Now cobra can initialize and add from within any Go module. The experience is simplified and streamlined, but requires `go mod init` to happen first.
c06749d
to
d7b9dc5
Compare
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.
@spf13 since the documentation is changing here could you also open a PR on https://github.com/spf13/cobra.dev if you haven't made the changes there already?
return mod, dir | ||
} | ||
|
||
type Mod struct { |
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.
nit: can you put each of these variables on their own line?
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 can. What is the motivation for doing so? (mostly just curious).
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.
For me it's readability. I find it easier to read the structure fields when they are not condensed on one line.
projectPath, err := initializeProject(args) | ||
cobra.CheckErr(err) | ||
cobra.CheckErr(goGet("github.com/spf13/cobra")) |
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'm not sure how I feel about this utility fetching dependencies for the user. @wfernandes @jpmcb thoughts here?
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 guess the point is having them explicitly added to the go.mod
file of the user? Not so much about fetching them.
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.
Yes, that is the intent. Russ Cox suggested we do this extra step and I agree. It streamlines the experience for the user as well as simplifying the instructions.
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.
One of the motivations for this update is that we are writing a tutorial on building CLIs with Go for golang.org / go.dev which uses Cobra. As we wrote the tutorial it became clear that there were lots of places in the onboarding experience that could be streamlined. This set of changes comes from those learnings.
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.
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.
See my note on #1240. I think we're likely to resolve this in the next few months by removing the need for cobra generator entirely.
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.
As we wrote the tutorial it became clear that there were lots of places in the onboarding experience that could be streamlined.
I'm inclined to agree on this point but am always wary of tools that try to do too many things.
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.
We can move forward with this change though.
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.
If this is just in the cobra CLI tool, sounds good to me. I'd be more cautious of anything in the API that would automatically fetch dependencies. But it makes sense to try and streamline the onboarding experience and get people up and going with cobra init
as quick as possible 👍
Merging the updated documentation from the user_guide into the cobra/README.md. Adding links as appropriate to both guides.
I'll do that as soon as this is accepted. For that, do you want a PR or for me to just make the changes directly? Happy to do either. |
@@ -3,41 +3,78 @@ | |||
Cobra provides its own program that will create your application and add any | |||
commands you want. It's the easiest way to incorporate Cobra into your application. | |||
|
|||
In order to use the cobra command, compile it using the following command: | |||
Install the cobra generator with the command `go install github.com/spf13/cobra/cobra`. |
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.
In CI, we are currently using GOBIN (see https://github.com/spf13/cobra/blob/master/.github/workflows/Test.yml). Might be worth adding a reference to https://golang.org/cmd/go/#hdr-Compile_and_install_packages_and_dependencies here?
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.
good idea.
Yes please PR it. |
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.
@spf13 are you ready for this to be merged?
I still need to write up the PR for the doc site. I've been swamped with
other stuff. I think that's all this is waiting on. It will be a couple
weeks before I'll be able to get to it. I'm fine if anyone else wants to do
that and merge this, but realistically speaking I'll be a couple weeks
before I can.
…On Wed, Jul 21, 2021 at 12:03 AM Joshua Harshman ***@***.***> wrote:
***@***.**** approved this pull request.
@spf13 <https://github.com/spf13> are you ready for this to be merged?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1436 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABKKZB2XQIAKJKKPIALHRLTYZBJPANCNFSM47VSJ6RA>
.
|
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.
Sorry for being late to this one - this looks good to me and I think will be a welcome change since most everyone should be on go modules by now! 🚀
This PR is being marked as stale due to a long period of inactivity |
I'll try to get to this in the next week or two. Bumping so it's not stale anymore. |
The
cobra
tool needed some extra updating to make it work seamlessly within the world of Go modules.I've dramatically simplified it to just work within a Go module. Now within a module you can just
cobra init
and it works. No paths or module names required.I've also made two additional behavioral changes: