Skip to content
This repository was archived by the owner on Jan 21, 2020. It is now read-only.

Conversation

wfarner
Copy link
Contributor

@wfarner wfarner commented Nov 15, 2016

I'd like to propose this style as a convention for declaring CLI flags and associated variables. I like this since it eliminates the data flow cycle we frequently had otherwise (declare a zeroed var, refer to zeroed var for flag default, refer to var pointer for arg value).

Signed-off-by: Bill Farner bill@docker.com

@wfarner
Copy link
Contributor Author

wfarner commented Nov 15, 2016

If we achieve consensus on this style, i will go ahead and update the rest of the codebase in this PR.

@codecov-io
Copy link

codecov-io commented Nov 15, 2016

Current coverage is 70.99% (diff: 0.00%)

Merging #301 into master will increase coverage by 0.11%

@@             master       #301   diff @@
==========================================
  Files            25         25          
  Lines          1243       1241     -2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            881        881          
+ Misses          287        285     -2   
  Partials         75         75          

Powered by Codecov. Last update 1519d12...70545ac

Short: "Access flavor plugin",
PersistentPreRunE: func(c *cobra.Command, args []string) error {
}
name := cmd.PersistentFlags().String("name", "", "Name of plugin")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really need to be a PersistentFlags()?
It doesn't seem that the name is passed down to any sub-children, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does need to be, as you want to specify a plugin name for most of the subcommands being used. IMHO it really should be a positional (required) argument rather than a flag, but that's not something i'm willing to tackle in this patch.

Copy link
Contributor

@FrenchBen FrenchBen left a comment

Choose a reason for hiding this comment

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

LGTM

@chungers
Copy link
Contributor

I think having pointer deferences in the function bodies look a bit awkward.

That said, if it's just a convention / pattern then I am ok with it.

@wfarner
Copy link
Contributor Author

wfarner commented Nov 15, 2016

@chungers i agree, but i see it as a lesser of evils. IMHO this pattern reduces likelihood of subtle bugs for a net improvement.

Signed-off-by: Bill Farner <bill@docker.com>
Signed-off-by: Bill Farner <bill@docker.com>
Signed-off-by: Bill Farner <bill@docker.com>
@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "arg-var" git@github.com:wfarner/infrakit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354593776
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Copy link
Contributor

@chungers chungers left a comment

Choose a reason for hiding this comment

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

LGTM

@wfarner wfarner merged commit 06ab6e7 into docker-archive:master Nov 17, 2016
@wfarner wfarner deleted the arg-var branch December 1, 2016 17:51
chungers pushed a commit to chungers/infrakit that referenced this pull request Sep 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants