bd, jhipster completions by Mazorius · Pull Request #4472 · fish-shell/fish-shell

Mazorius · 2017-10-12T08:26:52Z

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

Add bd, docker, docker-compose, jhipster completion

Add completion for docker

Add completion for docker-compose

Add completion for jhipster

floam · 2017-10-12T08:33:13Z

 - Option completion for `apt list` now works properly (#4350).
 - Added completions for:
  - `as` (#4130)
+  - `bd`


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

Ah thanks I will add the information.

Add PR id to changes.

floam · 2017-10-12T08:34:58Z

+# https://github.com/0rax/fish-bd
+#
+
+complete -c bd -s c -d "Classic mode : goes back to the first directory named as the string"


No space necessary before a colon.

floam · 2017-10-12T08:35:30Z

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


Try to use string match and string replace here.

floam · 2017-10-12T08:35:37Z

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


Try to use string match and string replace here.

Remove space in-front of colon.

floam · 2017-10-12T08:38:50Z

+complete -c bd -A -f
+
+function __fish_bd_complete_dirs
+	printf (pwd | sed 's|/|\\\n|g')


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

floam · 2017-10-12T08:40:32Z

+        '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]'


Try to use string match here.

floam · 2017-10-12T08:42:00Z

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

floam · 2017-10-12T08:43:09Z

Why are there two PRs for this?

Mazorius · 2017-10-12T08:44:47Z

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

I will fix your mentioned issues.

floam · 2017-10-12T08:46:41Z

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

Fix mentioned issues

- replace --description with -d - replace grep with string match - replace sed with string replace (expect sed -n)

floam · 2017-10-12T09:32:03Z

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


$PWD can be used here too.

Mazorius · 2017-10-12T09:33:23Z

@floam all requested changes are done.

Replace pwd command via echo $PWD

floam · 2017-10-12T09:35:57Z

+complete -c bd -A -f
+
+function __fish_bd_complete_dirs
+	printf (echo "$PWD" | string replace -a "/" "\n")


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

Remove parenthesis

floam · 2017-10-12T09:38:20Z


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


This is going to print the word echo.

remove parenthesis and echo command

fix issue inside printf

Mazorius · 2017-10-13T08:07:04Z

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

faho

Some wording suggestions for jhipster
A nit for bd
Please remove docker and docker-compose as they are shipped upstream by docker.

faho · 2017-10-13T08:19:12Z

+complete -c bd -A -f
+
+function __fish_bd_complete_dirs
+    printf $PWD | string replace -ar "/" "\n"


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

faho · 2017-10-13T08:20:59Z

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


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.

faho · 2017-10-13T08:22:09Z

+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.'


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 ".".

faho · 2017-10-13T08:23:00Z

+
+# 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.'


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

faho · 2017-10-13T08:24:11Z

+# 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.'


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

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

faho · 2017-10-13T08:26:36Z

+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..'


See the "app" comment above.

faho · 2017-10-13T08:27:43Z

+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.'


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?

faho · 2017-10-13T08:28:20Z

+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.'


Upgrade jhipster and the generated app?

faho · 2017-10-13T08:30:14Z

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.

Mazorius · 2017-10-13T17:26:16Z

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

But I can delete them.

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 · 2017-10-13T20:39:39Z

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

floam · 2017-10-13T21:13:39Z

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


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

Mazorius · 2017-10-15T12:37:08Z

@floam fixed mentioned issue

Mazorius · 2017-10-18T19:39:45Z

@faho @floam is everything alright now?

faho · 2017-11-13T09:59:18Z

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!

ridiculousfish · 2017-11-18T04:22:46Z

Any reason to not just do a squash merge?

faho · 2017-11-19T12:38:07Z

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.

ridiculousfish · 2017-11-27T03:22:56Z

I squashed this manually as ce4fdba and a4fced2 . Thanks!

Mazorius added 5 commits October 12, 2017 10:13

Update CHANGELOG.md

930fa0e

Add bd, docker, docker-compose, jhipster completion

Create bd.fish

0de352d

Create docker.fish

0817972

Add completion for docker

Create docker-compose.fish

a87b738

Add completion for docker-compose

Create jhipster.fish

96a38d7

Add completion for jhipster

floam changed the title ~~Add completions to 2.7.0~~ docker, docker-compose, bd, jhipster completions Oct 12, 2017

floam added this to the fish 2.7.0 milestone Oct 12, 2017

floam reviewed Oct 12, 2017

View reviewed changes

Update CHANGELOG.md

245c9ca

Add PR id to changes.

floam requested changes Oct 12, 2017

View reviewed changes

Update bd.fish

fa030d6

Remove space in-front of colon.

floam requested changes Oct 12, 2017

View reviewed changes

floam mentioned this pull request Oct 12, 2017

Add completions #4471

Closed

3 tasks

Mazorius added 2 commits October 12, 2017 10:58

Update bd.fish

08c653d

Fix mentioned issues

Update docker-compose.fish

1719035

- replace --description with -d - replace grep with string match - replace sed with string replace (expect sed -n)

floam reviewed Oct 12, 2017

View reviewed changes

Update docker-compose.fish

6727ad7

Replace pwd command via echo $PWD

floam reviewed Oct 12, 2017

View reviewed changes

Update bd.fish

0d84809

Remove parenthesis

floam reviewed Oct 12, 2017

View reviewed changes

Mazorius added 2 commits October 12, 2017 11:39

Update docker-compose.fish

e810b72

remove parenthesis and echo command

Update bd.fish

25e57fc

fix issue inside printf

faho requested changes Oct 13, 2017

View reviewed changes

floam changed the title ~~docker, docker-compose, bd, jhipster completions~~ bd, jhipster completions Oct 13, 2017

Ron Gebauer added 3 commits October 13, 2017 21:13

Remove docker completions.

57a8dfd

docker.fish and docker-compose.fish were removed.

Fix reviewed stuff

27eae73

- Add long version for options in jhipster.fish - Improve bd.fish as mentioned - format jhipster.fish - Improve jihpster.fish as mentioned

format bd.fish

cbe5830

floam requested changes Oct 13, 2017

View reviewed changes

Remove unneeded -a inside if clause.

0b66572

faho modified the milestones: fish 2.7.0, fish-3.0 Nov 23, 2017

floam approved these changes Nov 23, 2017

View reviewed changes

ridiculousfish closed this Nov 27, 2017

github-actions Bot locked as resolved and limited conversation to collaborators Apr 17, 2020

Uh oh!

Conversation

Mazorius commented Oct 12, 2017 • edited by floam Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODOs:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

floam commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

floam commented Oct 12, 2017

Uh oh!

Mazorius commented Oct 12, 2017

Uh oh!

floam commented Oct 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mazorius commented Oct 12, 2017

Uh oh!

floam Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mazorius commented Oct 13, 2017

Uh oh!

faho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

floam Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

faho commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mazorius commented Oct 13, 2017

Uh oh!

Mazorius commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mazorius commented Oct 15, 2017

Uh oh!

Mazorius commented Oct 18, 2017

Uh oh!

faho commented Nov 13, 2017

Uh oh!

Mazorius commented Oct 12, 2017 •

edited by floam

Loading

floam commented Oct 12, 2017 •

edited

Loading

floam Oct 12, 2017 •

edited

Loading

floam Oct 13, 2017 •

edited

Loading

faho commented Oct 13, 2017 •

edited

Loading