Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

rkt: bash completion code #3774

Merged
merged 1 commit into from
Sep 4, 2017
Merged

rkt: bash completion code #3774

merged 1 commit into from
Sep 4, 2017

Conversation

ybubnov
Copy link
Contributor

@ybubnov ybubnov commented Aug 13, 2017

This patch provides an implementation of the command used to generate completion code for the bash shell. This feature is extremely useful for distributions of rkt that does not provide bash completion files and can be easily generated by the user instead.

closes #3731

This patch provides an implementation of the command used to generate
completion code for the bash shell. This feature is extremely useful
for distributions of rkt that does not provide bash completion files
and can be easily generated by the user instead.
Run: runWrapper(newCompletion(os.Stdout)),
}

bashCompletionFunc = `__rkt_parse_image()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use bash_completion_func as this naming convention is more often used in sh world.

Copy link
Contributor Author

@ybubnov ybubnov Aug 14, 2017

Choose a reason for hiding this comment

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

I'm simply trying to follow Go naming convention for multiword identifiers, which dictates to use mixed caps instead of underscores: https://golang.org/doc/effective_go.html#mixed-caps.

Do you want me change the identifier back to what is commonly used?

Copy link
Contributor

Choose a reason for hiding this comment

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

But this is not a golang code, it's bash code, so sh naming convention should be used, instead of linked golang one.
One of better references: https://google.github.io/styleguide/shell.xml?showone=Function_Names#Function_Names

Yes, IMO you should change the identifier back to what was used before.

Copy link
Member

Choose a reason for hiding this comment

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

@jellonek I'm not sure I understood what you are saying. This line seems to be defining a golang string variable, which should follow the usual naming convention as pointed out by @ybubnov. What the variable contains is irrelevant to golang, it is just a private string.

Copy link
Contributor

Choose a reason for hiding this comment

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

... very strong facepalm on my side - i misreaded the diff, ybubnov is right that this should be camel case, so it's a good correction later above in rkt/rkt.go...

Sorry for unnecessary mistake...

@@ -153,7 +97,7 @@ var cmdRkt = &cobra.Command{
Long: `A CLI for running app containers on Linux.

To get the help on any specific command, run "rkt help command".`,
BashCompletionFunction: bash_completion_func,
BashCompletionFunction: bashCompletionFunc,
Copy link
Contributor

Choose a reason for hiding this comment

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

So this change will be unnecessary.

@ybubnov
Copy link
Contributor Author

ybubnov commented Sep 1, 2017

Hi, sorry, for being noisy, is something required from my side for this pull-request?

Copy link
Member

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

lgtm

@iaguis iaguis merged commit c4d5165 into rkt:master Sep 4, 2017
@iaguis
Copy link
Member

iaguis commented Sep 4, 2017

Thanks!

@ybubnov ybubnov deleted the rkt-bash-completion branch September 4, 2017 11:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rkt embedded bash-completions
4 participants