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

fix color completition #2836

Merged
merged 2 commits into from Jul 31, 2019
Merged

Conversation

edusantana
Copy link
Contributor

see #2835

Short release notes: Fix bash completion
Longer description: Now you can call kdb and it will return

check          fstab          import         mount          shell
complete       get            info           mv             smount
convert        getmeta        list           remount        spec-mount
cp             global-mount   list-commands  rm             test
editor         global-umount  list-tools     rmmeta         umount
export         gmount         ls             set            vset
file           gumount        lsmeta         setmeta        
find           help           merge          sget

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Fix Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see Coding Guidelines)
  • meta data is updated (e.g. README.md of plugins and METADATA.ini)

Review

Reviewers will usually check the following:

Labels

  • Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • Add the "ready to merge" label if the build server is happy and also you
    say that everything is ready to be merged.

@markus2330
Copy link
Contributor

Thank you for your contribution!

Please, as stated by the build server, write a line in doc/news/_preparation_next_release.md to make sure your contribution will be mentioned in the next release notes.

Furthermore, there was a formatting problem (we use automatic code formatters) which is fixed in #2837.

Copy link
Member

@sanssecours sanssecours left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request. I just added one code suggestion that might simplify the updated line. At least on my machine the simplified code seems to work too. Please feel free to ignore the suggestion, if you do not like it.

@@ -23,7 +23,7 @@ _kdb() {
# only kdb was entered yet, print a list of available commands
if [[ $COMP_CWORD -eq 1 ]]; then
local IFS=$'\n'
local commands=($(${kdbpath} 2> /dev/null | sed -e '0,/^Known commands are/d' | awk '{print $1}'))
local commands=($(${kdbpath} 2> /dev/null | sed -e '0,/^Known commands are/d' | awk '{print $1}' | sed -r "s/\x1B\[([0-9]{1,3}((;[0-9]{1,3})*)?)?[m|K]//g" ))
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use the kdb subcommand list-commands to simplify this code:

Suggested change
local commands=($(${kdbpath} 2> /dev/null | sed -e '0,/^Known commands are/d' | awk '{print $1}' | sed -r "s/\x1B\[([0-9]{1,3}((;[0-9]{1,3})*)?)?[m|K]//g" ))
local commands=($(${kdbpath} list-commands))

.

@edusantana
Copy link
Contributor Author

@sanssecours that's even better!

@markus2330 markus2330 merged commit da40250 into ElektraInitiative:master Jul 31, 2019
@markus2330
Copy link
Contributor

Thank you for your contribution! We hope to see more from you soon!

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

3 participants