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

allow bash completion to complete tasks in other directories #1303

Merged
merged 10 commits into from
Sep 24, 2022

Conversation

jpbochi
Copy link
Contributor

@jpbochi jpbochi commented Aug 4, 2022

Today, I learned about just's ability to invoke justfiles in other directories.

I moved part of my tasks into ./ci/justfile. Then, with bash completion on, typed just ci/ + tab. It completed with files, not with just tasks.

This change here solves that problem.

If the current text being completed has a / char, we'll run a different just --summary command.

completions/just.bash Outdated Show resolved Hide resolved
completions/just.bash Outdated Show resolved Hide resolved
@jpbochi jpbochi force-pushed the jp/completion branch 2 times, most recently from c2979e7 to 637ead1 Compare August 10, 2022 08:36
@jpbochi
Copy link
Contributor Author

jpbochi commented Aug 16, 2022

@casey Can you take a look at this one again? I believe I addressed all your previous points.

@casey
Copy link
Owner

casey commented Aug 18, 2022

Sorry for the delay! Just checked it out, and I think there's a bug. One the left is the result of me typing just <tab> in the PR branch, and on the right is me typing just <tab> on master. Both of these are after having done source completions/just.bash to pick up the completion script. It looks like something is going wrong with the terminal width detection in the PR branch's completion script.

Screen Shot 2022-08-17 at 10 40 18 PM

@jpbochi
Copy link
Contributor Author

jpbochi commented Aug 18, 2022

@casey That's very odd. A bash completion only writes to the COMPREPLY variable. It's up to bash itself to deal with terminal width.

Here are some tests I did from this branch:

$ source ./completions/just.bash

$ COMP_WORDS=(just) && COMP_CWORD=1 _just just && declare -p COMPREPLY
declare -a COMPREPLY=([0]="build" [1]="build-book" [2]="changes" [3]="check" [4]="ci" [5]="clippy" [6]="done" [7]="filter" [8]="fmt" [9]="forbid" [10]="fuzz" [11]="generate-completions" [12]="install" [13]="install-dev-deps" [14]="install-dev-deps-homebrew" [15]="man" [16]="polyglot" [17]="pr" [18]="publish" [19]="push" [20]="pwd" [21]="quine" [22]="render-readme" [23]="replace" [24]="run" [25]="sloc" [26]="test" [27]="test-quine" [28]="view-man" [29]="watch" [30]="watch-readme")

$ COMP_WORDS=(just i) && COMP_CWORD=1 _just just && declare -p COMPREPLY
declare -a COMPREPLY=([0]="install" [1]="install-dev-deps" [2]="install-dev-deps-homebrew")

$ COMP_WORDS=(just r) && COMP_CWORD=1 _just just && declare -p COMPREPLY
declare -a COMPREPLY=([0]="render-readme" [1]="replace" [2]="run")

And now with just 1.4.0:

$ eval "$(just --completions bash)"

$ COMP_WORDS=(just) && COMP_CWORD=1 _just just && declare -p COMPREPLY
declare -a COMPREPLY=([0]="build" [1]="build-book" [2]="changes" [3]="check" [4]="ci" [5]="clippy" [6]="done" [7]="filter" [8]="fmt" [9]="forbid" [10]="fuzz" [11]="generate-completions" [12]="install" [13]="install-dev-deps" [14]="install-dev-deps-homebrew" [15]="man" [16]="polyglot" [17]="pr" [18]="publish" [19]="push" [20]="pwd" [21]="quine" [22]="render-readme" [23]="replace" [24]="run" [25]="sloc" [26]="test" [27]="test-quine" [28]="view-man" [29]="watch" [30]="watch-readme")

$ COMP_WORDS=(just i) && COMP_CWORD=1 _just just && declare -p COMPREPLY
declare -a COMPREPLY=([0]="install" [1]="install-dev-deps" [2]="install-dev-deps-homebrew")

$ COMP_WORDS=(just r) && COMP_CWORD=1 _just just && declare -p COMPREPLY
declare -a COMPREPLY=([0]="render-readme" [1]="replace" [2]="run")

The version of bash I'm using is GNU bash, version 5.1.16(1)-release (aarch64-apple-darwin21.1.0), for the record.

@jpbochi jpbochi closed this Aug 18, 2022
@jpbochi jpbochi reopened this Aug 18, 2022
@jpbochi
Copy link
Contributor Author

jpbochi commented Aug 18, 2022

I found a different bug, and fixed it.

$ mkdir ci

$ echo 'install:' >> ci/justfile

$ echo 'deploy:' >> ci/justfile

$ source ./completions/just.bash && COMP_WORDS=(just ci/) && COMP_CWORD=1 _just just && declare -p COMPREPLY
declare -a COMPREPLY=([0]="ci/deploy" [1]="ci/install")

$ echo 'another:' >> ci/justfile

$ source ./completions/just.bash && COMP_WORDS=(just ci/) && COMP_CWORD=1 _just just && declare -p COMPREPLY
declare -a COMPREPLY=([0]="ci/another" [1]="ci/deploy" [2]="ci/install")

It would be great to add those as unit tests somehow.

@casey
Copy link
Owner

casey commented Aug 18, 2022

If you could add unit tests for this, that would be amazing. So far I haven't because I'm not sure how to do that 😅

@indigoviolet
Copy link

Sorry if this is a tangent, but I noticed that the completion script won't complete filenames in some contexts: for example, after just -f<TAB> or just --dotenv-filename<TAB>. Is it possible to ensure that always happens?

@jpbochi
Copy link
Contributor Author

jpbochi commented Aug 23, 2022

I've used BATS (https://bats-core.readthedocs.io/) in the past. It's focused on bash, instead of zsh or plain sh, but it may be flexible enough to allow tests for other shells, too. It has been in active development for many years.

A recent alternative is https://shellspec.info, but I'm not too familiar with it. Its "BDD" API seems pretty weird to me.

One alternative is to write our own test script for now.

@jpbochi
Copy link
Contributor Author

jpbochi commented Aug 23, 2022

@casey I added a custom script to test completion of bash. That could be extended later for other shells. And could be converted to BATS, if needed.

@jpbochi
Copy link
Contributor Author

jpbochi commented Aug 24, 2022

for future reference, I found out abotu that way to test bash completion here: https://stackoverflow.com/questions/3520936/accessing-bash-completions-for-specific-commands-programmatically

@casey
Copy link
Owner

casey commented Sep 3, 2022

Sorry for the delay once again! Taking a look again, and the tests seem to still be failing locally for me:

: ./tests/completions/just.bash                                                                                                                                                                                      ~/src/just ⟦jp/completion⟧
test_just_is_accessible: ok
test_complete_all_recipes: failed! Completion for `just` does not match.

--- expected
+++ actual
@@ -1 +1 @@
-declare -a COMPREPLY=([0]="build" [1]="build-book" [2]="changes" [3]="check" [4]="ci" [5]="clippy" [6]="done" [7]="filter" [8]="fmt" [9]="forbid" [10]="fuzz" [11]="generate-completions" [12]="install" [13]="install-dev-deps" [14]="install-dev-deps-homebrew" [15]="man" [16]="polyglot" [17]="pr" [18]="publish" [19]="push" [20]="pwd" [21]="quine" [22]="render-readme" [23]="replace" [24]="run" [25]="sloc" [26]="test" [27]="test-quine" [28]="view-man" [29]="watch" [30]="watch-readme")
+declare -a COMPREPLY='([0]="build" [1]="build-book" [2]="changes" [3]="check" [4]="ci" [5]="clippy" [6]="done" [7]="filter" [8]="fmt" [9]="forbid" [10]="fuzz" [11]="generate-completions" [12]="install" [13]="install-dev-deps" [14]="install-dev-deps-homebrew" [15]="man" [16]="polyglot" [17]="pr" [18]="publish" [19]="push" [20]="pwd" [21]="quine" [22]="render-readme" [23]="replace" [24]="run" [25]="sloc" [26]="test" [27]="test-quine" [28]="view-man" [29]="watch" [30]="watch-readme")'

test_complete_recipes_starting_with_i: failed! Completion for `just i` does not match.

--- expected
+++ actual
@@ -1 +1 @@
-declare -a COMPREPLY=([0]="install" [1]="install-dev-deps" [2]="install-dev-deps-homebrew")
+declare -a COMPREPLY='([0]="install" [1]="install-dev-deps" [2]="install-dev-deps-homebrew")'

test_complete_recipes_starting_with_r: failed! Completion for `just r` does not match.

--- expected
+++ actual
@@ -1 +1 @@
-declare -a COMPREPLY=([0]="render-readme" [1]="replace" [2]="run")
+declare -a COMPREPLY='([0]="render-readme" [1]="replace" [2]="run")'

test_complete_recipes_from_subdirs: failed! Completion for `just tmp/` does not match.

--- expected
+++ actual
@@ -1 +1 @@
-declare -a COMPREPLY=([0]="tmp/deploy" [1]="tmp/install")
+declare -a COMPREPLY='([0]="tmp/deploy" [1]="tmp/install")'

Some test[s] failed.

Also, the tests should run against a test justfile, since otherwise the tests will need to be updated every time the main justfile changes.

@jpbochi
Copy link
Contributor Author

jpbochi commented Sep 6, 2022

@casey interesting... My guess is that this is caused by a different bash version. Which bash --version do you have?

I'm on GNU bash, version 5.1.16(1)-release (aarch64-apple-darwin21.1.0), while CI has GNU bash, version 5.0.17(1)-release (x86_64-pc-linux-gnu). And those tests pass on both of them.

Naturally, I'll try to adapt them to pass regardless.

@casey
Copy link
Owner

casey commented Sep 6, 2022

Ah, good point. I have the super old bash that ships with MacOS. I installed a newer one and it worked, so don't worry about making it pass with the old version.

@jpbochi
Copy link
Contributor Author

jpbochi commented Sep 7, 2022

@casey This should be good to be merged then. The change works (for me , at least), and tests are passing.

I think that code should even work on the older bash you had before. The issue was only on the test diff.

@jpbochi
Copy link
Contributor Author

jpbochi commented Sep 20, 2022

@casey can you take a look at this again? I think it's ready to be merged, unless you have some pending concerns I missed.

@casey
Copy link
Owner

casey commented Sep 21, 2022

Sorry for letting this sit for so long! Looks good, although can you make it use a dummy justfile that's only used for the test, instead of the project's justfile? If it uses the main justfile, then it will break when it changes.

@jpbochi
Copy link
Contributor Author

jpbochi commented Sep 21, 2022

... can you make it use a dummy justfile that's only used for the test, instead of the project's justfile? If it uses the main justfile, then it will break when it changes.

sounds reasonable. I'll make the change.

@jpbochi
Copy link
Contributor Author

jpbochi commented Sep 21, 2022

@casey updated. Tests don't rely on the main justfile anymore.

@casey
Copy link
Owner

casey commented Sep 22, 2022

Nice! Overall looks good. Instead of creating a tempdir and temp justfile, how about committing the test justfile to tests/completions and cd-ing into that directory to perform the tests?

@jpbochi
Copy link
Contributor Author

jpbochi commented Sep 22, 2022

@casey done. I created and commited two test justfiles, and used those. :)

@casey casey merged commit 7f72755 into casey:master Sep 24, 2022
@casey
Copy link
Owner

casey commented Sep 24, 2022

Nice, merged! Thanks for sticking with this!

@jpbochi jpbochi deleted the jp/completion branch October 3, 2022 12:20
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.

3 participants