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

bd, jhipster completions #4472

Conversation

@Mazorius
Copy link
Contributor

@Mazorius Mazorius commented Oct 12, 2017

Description

Added completions are:

  • docker
  • docker-compose
  • bd
  • jhipster

Fixes issue: no issue

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
Do not know if needed and where to put it.
  • Tests have been added for regressions fixed
No tests needed because of changes only happens inside completion.
If I am wrong please give me an advise
  • User-visible changes noted in CHANGELOG.md
Mazorius added 5 commits Oct 12, 2017
Add bd, docker, docker-compose, jhipster completion
Add completion for docker
Add completion for docker-compose
Add completion for jhipster
@floam floam changed the title Add completions to 2.7.0 docker, docker-compose, bd, jhipster completions Oct 12, 2017
@floam floam added this to the fish 2.7.0 milestone Oct 12, 2017
CHANGELOG.md Outdated
@@ -31,13 +31,17 @@
- Option completion for `apt list` now works properly (#4350).
- Added completions for:
- `as` (#4130)
- `bd`
Copy link
Member

@floam floam Oct 12, 2017

Choose a reason for hiding this comment

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

You can cite the number of this PR. (I understand you'd be hard-pressed to figure it out before submitting!)

Loading

Copy link
Contributor Author

@Mazorius Mazorius Oct 12, 2017

Choose a reason for hiding this comment

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

Ah thanks I will add the information.

Loading

Add PR id to changes.
# https://github.com/0rax/fish-bd
#

complete -c bd -s c -d "Classic mode : goes back to the first directory named as the string"
Copy link
Member

@floam floam Oct 12, 2017

Choose a reason for hiding this comment

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

No space necessary before a colon.

Loading


switch $file_version
case '1'
cat $path | command grep '^[a-zA-Z]' | command sed 's/://'
Copy link
Member

@floam floam Oct 12, 2017

Choose a reason for hiding this comment

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

Try to use string match and string replace here.

Loading

# 2 spaces. Make it work with any indentation.
cat $path | command sed -n '/^services:/,/^\w/p' \
| command grep "^ [$chars]*:" \
| command sed "s/[^$chars]//g"
Copy link
Member

@floam floam Oct 12, 2017

Choose a reason for hiding this comment

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

Try to use string match and string replace here.

Loading

Remove space in-front of colon.
complete -c bd -A -f

function __fish_bd_complete_dirs
printf (pwd | sed 's|/|\\\n|g')
Copy link
Member

@floam floam Oct 12, 2017

Choose a reason for hiding this comment

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

Try to use string replace here, and $PWD instead of the pwd command.

Loading

'Get the version of a docker-compose.yml file.'
cat (__fish_docker_compose_file_path) \
| command grep '^version:\(\s*\)["\']\?[0-9]["\']\?' \
| command grep -o '[0-9]'
Copy link
Member

@floam floam Oct 12, 2017

Choose a reason for hiding this comment

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

Try to use string match here.

Loading

@floam
Copy link
Member

@floam floam commented Oct 12, 2017

Thanks! I found a few things that might be addressed. @faho may be able to give further advice or a thumbs up.

Loading

@floam
Copy link
Member

@floam floam commented Oct 12, 2017

Why are there two PRs for this?

Loading

@Mazorius
Copy link
Contributor Author

@Mazorius Mazorius commented Oct 12, 2017

@floam I added changes for 2.7.0 and 3.0.0
What is the better way?

I will fix your mentioned issues.

Loading

@floam
Copy link
Member

@floam floam commented Oct 12, 2017

We only need one PR. We'll merge it where necessary.

Loading

@floam floam mentioned this pull request Oct 12, 2017
3 tasks
Mazorius added 2 commits Oct 12, 2017
Fix mentioned issues
- replace --description with -d
- replace grep with string match
- replace sed with string replace (expect sed -n)

function __fish_docker_compose_file_path -d \
'Get the next docker-compose.yml file in the folder parent path.'
set -l path (pwd)
Copy link
Member

@floam floam Oct 12, 2017

Choose a reason for hiding this comment

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

$PWD can be used here too.

Loading

@Mazorius
Copy link
Contributor Author

@Mazorius Mazorius commented Oct 12, 2017

@floam all requested changes are done.

Loading

Replace pwd command via echo $PWD
complete -c bd -A -f

function __fish_bd_complete_dirs
printf (echo "$PWD" | string replace -a "/" "\n")
Copy link
Member

@floam floam Oct 12, 2017

Choose a reason for hiding this comment

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

Can just be: printf $PWD | string replace -a "/" "\n"

Loading

floam
Copy link
Member

floam commented on 6727ad7 Oct 12, 2017

Choose a reason for hiding this comment

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

Can just be: set -l path $PWD

Loading

Remove parenthesis
@@ -12,7 +12,7 @@ complete -c bd -s h -x -d "Display help and exit"
complete -c bd -A -f

function __fish_bd_complete_dirs
printf (echo "$PWD" | string replace -a "/" "\n")
printf echo "$PWD" | string replace -a "/" "\n"
Copy link
Member

@floam floam Oct 12, 2017

Choose a reason for hiding this comment

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

This is going to print the word echo.

Loading

Mazorius added 2 commits Oct 12, 2017
remove parenthesis and echo command
fix issue inside printf
@Mazorius
Copy link
Contributor Author

@Mazorius Mazorius commented Oct 13, 2017

I think it should work now.
What do you think @floam

Loading

Copy link
Member

@faho faho left a comment

  • Some wording suggestions for jhipster

  • A nit for bd

  • Please remove docker and docker-compose as they are shipped upstream by docker.

Loading

complete -c bd -A -f

function __fish_bd_complete_dirs
printf $PWD | string replace -ar "/" "\n"
Copy link
Member

@faho faho Oct 13, 2017

Choose a reason for hiding this comment

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

This reads better as string split "/" -- $PWD

Loading

function __fish_prog_needs_command
set cmd (commandline -opc)
echo $cmd
if [ (count $cmd) -eq 1 -a $cmd[1] = 'jhipster' ]
Copy link
Member

@faho faho Oct 13, 2017

Choose a reason for hiding this comment

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

Please remove the $cmd[1] = 'jhipster' part - that breaks if you use an alias or any other way of "wrapping" this. It's also completely unnecessary, since fish has already determined that jhipster should be completed.

Loading

complete -f -c jhipster -n '__fish_prog_needs_command' -s V -d 'Output version number'

# Commands
complete -f -c jhipster -n '__fish_prog_needs_command' -a app -d 'Create a new JHipster application based on the selected options.'
Copy link
Member

@faho faho Oct 13, 2017

Choose a reason for hiding this comment

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

Just Create a new app? (Note: I know exactly nothing about jhipster)

That it's a JHipster thing is implicit since you're executing jhipster, and that it's based on the selected options oes without saying.

Also, please no trailing ".".

Loading


# Commands
complete -f -c jhipster -n '__fish_prog_needs_command' -a app -d 'Create a new JHipster application based on the selected options.'
complete -f -c jhipster -n '__fish_prog_needs_command' -a aws -d 'Deploy the current application to Amazon Web Services.'
Copy link
Member

@faho faho Oct 13, 2017

Choose a reason for hiding this comment

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

I'd imagine "AWS" is well-known enough that we can use the acronym.

Loading

# Commands
complete -f -c jhipster -n '__fish_prog_needs_command' -a app -d 'Create a new JHipster application based on the selected options.'
complete -f -c jhipster -n '__fish_prog_needs_command' -a aws -d 'Deploy the current application to Amazon Web Services.'
complete -f -c jhipster -n '__fish_prog_needs_command' -a ci-cd -d 'Create pipeline scripts for popular Continuous Integration/Continuous Deployment tools.'
Copy link
Member

@faho faho Oct 13, 2017

Choose a reason for hiding this comment

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

No need to say "continuous" twice - Create pipeline scripts for Continuous Integration/Deployment tools?

Loading

Copy link
Member

@floam floam Oct 13, 2017

Choose a reason for hiding this comment

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

Maybe try to shorten further - "continuous integration/deployment tools" - or just "CI tools"?

Loading

complete -f -c jhipster -n '__fish_prog_needs_command' -a app -d 'Create a new JHipster application based on the selected options.'
complete -f -c jhipster -n '__fish_prog_needs_command' -a aws -d 'Deploy the current application to Amazon Web Services.'
complete -f -c jhipster -n '__fish_prog_needs_command' -a ci-cd -d 'Create pipeline scripts for popular Continuous Integration/Continuous Deployment tools.'
complete -f -c jhipster -n '__fish_prog_needs_command' -a client -d 'Create a new JHipster client-side application based on the selected options..'
Copy link
Member

@faho faho Oct 13, 2017

Choose a reason for hiding this comment

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

See the "app" comment above.

Loading

complete -f -c jhipster -n '__fish_prog_needs_command' -a aws -d 'Deploy the current application to Amazon Web Services.'
complete -f -c jhipster -n '__fish_prog_needs_command' -a ci-cd -d 'Create pipeline scripts for popular Continuous Integration/Continuous Deployment tools.'
complete -f -c jhipster -n '__fish_prog_needs_command' -a client -d 'Create a new JHipster client-side application based on the selected options..'
complete -f -c jhipster -n '__fish_prog_needs_command' -a cloudfoundry -d 'Generate a `deploy/cloudfoundry` folder with a specific manifest.yml to deploy to Cloud Foundry.'
Copy link
Member

@faho faho Oct 13, 2017

Choose a reason for hiding this comment

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

Is it necessary to mention manifest.yml here?

I.e. would it be obvious to anyone who knows this tool that this reads manifest.yml?

Loading

complete -f -c jhipster -n '__fish_prog_needs_command' -a rancher-compose -d 'Deploy the current application to Rancher.'
complete -f -c jhipster -n '__fish_prog_needs_command' -a server -d 'Create a new JHipster server-side application.'
complete -f -c jhipster -n '__fish_prog_needs_command' -r -a service -d 'Create a new Spring service bean.'
complete -f -c jhipster -n '__fish_prog_needs_command' -a upgrade -d 'Upgrade the JHipster version, and upgrade the generated application.'
Copy link
Member

@faho faho Oct 13, 2017

Choose a reason for hiding this comment

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

Upgrade jhipster and the generated app?

Loading

@faho
Copy link
Member

@faho faho commented Oct 13, 2017

Completions for both docker and docker-compose are upstream, so we should not ship them.

If you have made any improvements to these, please add them there instead of here.

Loading

@floam floam changed the title docker, docker-compose, bd, jhipster completions bd, jhipster completions Oct 13, 2017
@Mazorius
Copy link
Contributor Author

@Mazorius Mazorius commented Oct 13, 2017

@faho both completions are upstream but not automatically installed on mac and not managed on docker doc.

But I can delete them.

Loading

Ron Gebauer added 3 commits Oct 13, 2017
docker.fish and docker-compose.fish were removed.
- Add long version for options in jhipster.fish
- Improve bd.fish as mentioned
- format jhipster.fish
- Improve jihpster.fish as mentioned
@Mazorius
Copy link
Contributor Author

@Mazorius Mazorius commented Oct 13, 2017

@faho @floam all mentioned stuff is fixed :-)

Loading

function __fish_prog_needs_command
set cmd (commandline -opc)
echo $cmd
if [ (count $cmd) -eq 1 -a ]
Copy link
Member

@floam floam Oct 13, 2017

Choose a reason for hiding this comment

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

-a is for combining with another expression. Since you removed the other part it is doing nothing. It should go.

Loading

@Mazorius
Copy link
Contributor Author

@Mazorius Mazorius commented Oct 15, 2017

@floam fixed mentioned issue

Loading

@Mazorius
Copy link
Contributor Author

@Mazorius Mazorius commented Oct 18, 2017

@faho @floam is everything alright now?

Loading

@faho
Copy link
Member

@faho faho commented Nov 13, 2017

I'm okay with merging this now, but if we want to have it nice and clean, it's going to need a more complicated merge, and it's gonna be a while before I have enough time to do that.

If anyone beats me to it, have at it!

Loading

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Nov 18, 2017

Any reason to not just do a squash merge?

Loading

@faho
Copy link
Member

@faho faho commented Nov 19, 2017

Any reason to not just do a squash merge?

@ridiculousfish: It's two different completions. I mean it's pretty unlikely that we'd want to revert one of them, but it's a bit unclean.

Loading

@faho faho removed this from the fish 2.7.0 milestone Nov 23, 2017
@faho faho added this to the fish-3.0 milestone Nov 23, 2017
floam
floam approved these changes Nov 23, 2017
@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Nov 27, 2017

I squashed this manually as ce4fdba and a4fced2 . Thanks!

Loading

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants