Skip to content
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

Merged
merged 8 commits into from Nov 3, 2021
Merged

Module aware generator #1436

merged 8 commits into from Nov 3, 2021

Conversation

spf13
Copy link
Owner

@spf13 spf13 commented Jul 1, 2021

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:

  1. Viper is no longer included by default. I've heard too much feedback from users that are confused by this and don't know where Cobra ends and Viper begins. Let's simplify that experience.
  2. There is no default license. There's questionable legal ramifications with choosing a license for users without their election. Better to have them take an action to set the license.

spf13 added 3 commits July 1, 2021 17:16
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.
@CLAassistant
Copy link

CLAassistant commented Jul 1, 2021

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the area/cli [deprecated] For the cobra CLI !!! moved to spf13/cobra-cli label Jul 1, 2021
spf13 added 2 commits July 1, 2021 18:38
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.
cobra/cmd/init.go Show resolved Hide resolved
cobra/cmd/init.go Outdated Show resolved Hide resolved
user_guide.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@jharshman jharshman left a 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?

cobra/cmd/init.go Show resolved Hide resolved
return mod, dir
}

type Mod struct {
Copy link
Collaborator

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?

Copy link
Owner Author

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

Copy link
Collaborator

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"))
Copy link
Collaborator

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?

Copy link
Contributor

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.

Copy link
Owner Author

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.

Copy link
Owner Author

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.

Copy link
Contributor

@umarcor umarcor Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spf13, in the context of writing a tutorial and streamlining the experience for newcommers, you might want to have a look at #1240. I do think that might be a very disruptive change, but at the same time it would be very valuable to have your feedback.

That is also the purpose of #1430.

Copy link
Owner Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.
@spf13
Copy link
Owner Author

spf13 commented Jul 2, 2021

@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?

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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea.

@jharshman
Copy link
Collaborator

@spf13

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.

Yes please PR it.

Copy link
Collaborator

@jharshman jharshman left a 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?

@spf13
Copy link
Owner Author

spf13 commented Jul 21, 2021 via email

Copy link
Collaborator

@jpmcb jpmcb left a 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! 🚀

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

This PR is being marked as stale due to a long period of inactivity

@spf13
Copy link
Owner Author

spf13 commented Oct 5, 2021

I'll try to get to this in the next week or two. Bumping so it's not stale anymore.

cobra/README.md Outdated Show resolved Hide resolved
cobra/README.md Outdated Show resolved Hide resolved
cobra/README.md Outdated Show resolved Hide resolved
cobra/README.md Outdated Show resolved Hide resolved
cobra/README.md Outdated Show resolved Hide resolved
@spf13 spf13 merged commit bfacc59 into master Nov 3, 2021
@jpmcb jpmcb deleted the module-aware-generator branch October 24, 2022 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli [deprecated] For the cobra CLI !!! moved to spf13/cobra-cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants