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

Pass extra arguments to generator #942

Closed
wants to merge 7 commits into from
Closed

Pass extra arguments to generator #942

wants to merge 7 commits into from

Conversation

jsimonetti
Copy link

@jsimonetti jsimonetti commented Nov 30, 2016

This change will allow arguments to goagen which are added after the '--' argument to be passed allong to the generator.

Only full flags (such as --flag=value) are passed, because these flags have not been parsed yet. Short flags (such as -f value) are not passed.
A boolean flag should be added in the form of --flag=true.

The generate command becomes goagen gen -d <design> --pkg-path=<generator> -- --custom-arg=value

Fixes #934

Jeroen Simonetti added 3 commits November 30, 2016 03:59
This change will allow arguments to goagen which are add after the '--' argument to be passed allong to the generator.

Only full flags (such as --flag=value) are passed, because these flags have not been parsed yet. Short flags (such as -f value) are not passed.
A boolean flag should be added in the form of --flag=true.

Signed-off-by: Jeroen Simonetti <jeroen@simonetti.nl>
Visitting all flags will also pass all flags to the generator. This breaks all generators.

Signed-off-by: Jeroen Simonetti <jeroen@simonetti.nl>
Signed-off-by: Jeroen Simonetti <jeroen@simonetti.nl>
@raphael
Copy link
Member

raphael commented Nov 30, 2016

Thank you for the PR! It would be nice to avoid the custom flag parsing... Could we instead take what comes after the -- and plop it verbatim at the end of the command line we build for the custom generator? this way custom commands, shorthand flags etc. would just work.

@jsimonetti
Copy link
Author

Absolutely. I Just wanted to use the existing Flags map in the meta.Generator struct to keep the changes limited to main.
We can add a CustomFlags []string property to meta.Generator, which can hold everything after --

@raphael
Copy link
Member

raphael commented Nov 30, 2016

Great! Why a slice though? Shouldn't it just be a string? (Maybe I should look at what cobra does to understand better...)

@jsimonetti
Copy link
Author

Basically a slice only because this is the type cobra passes along.
It makes sence in a way because os.Args is a slice too. So if we would want to do extra parsing in meta.Generator in the future we could without extra splitting. If you prefer a string thats fine by me too.
I suppose it depends on if you want the strings.Join to happen in main or somewhere in meta.Generator, either wat, I have no oppinion.

@raphael
Copy link
Member

raphael commented Nov 30, 2016

Got it, a slice makes sense then, thanks for the clarification!

Jeroen Simonetti added 2 commits December 1, 2016 08:55
This will remove the flag parsing in favor of appending everything after -- to the final generator command arguments.

Signed-off-by: Jeroen Simonetti <jeroen@simonetti.nl>
@jsimonetti
Copy link
Author

If the final result of this is ok, I will squash everything together in one commit and resubmit the PR, just to make everything more readable.

Copy link
Member

@raphael raphael left a comment

Choose a reason for hiding this comment

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

Looks great!

Do you think you could add a test as well?

@@ -33,6 +33,11 @@ type Generator struct {
// generator on the command line.
Flags map[string]string

// CustomFlags is a list of arguments given to the final generator that
// where after the '--' argument and thus where not pardes by goagen.

This comment was marked as off-topic.

@jsimonetti
Copy link
Author

jsimonetti commented Dec 2, 2016

New PR #947

@jsimonetti jsimonetti closed this Dec 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants