-
-
Notifications
You must be signed in to change notification settings - Fork 315
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 #1404, and more... #1410
Fix #1404, and more... #1410
Conversation
If you miss the completion for the query filters that I have deleted from case $query in
acct:) hledgerArgs=(accounts --flat) ;;
code:) hledgerArgs=(codes) ;;
cur:) hledgerArgs=(commodities) ;;
desc:) hledgerArgs=(descriptions) ;;
note:) hledgerArgs=(notes) ;;
payee:) hledgerArgs=(payees) ;;
tag:) hledgerArgs=(tags) ;;
*)
local wordlist
case $query in
amt:) wordlist="< <= > >=" ;;
real:) wordlist="\ 0" ;;
status:) wordlist="\ * !" ;;
*) return 1 ;;
esac
_get_comp_words_by_ref -n '<=>' -c wordToComplete
_hledger_compreply "$(compgen -P "$query" -W "$wordlist" -- "${wordToComplete#*:}")"
return 0
;;
esac I am not convinced of its usefulness but just saying that they could be added back easily if deemed necessary. |
Also, what about this loop: hledger/shell-completion/hledger-completion.bash Lines 276 to 296 in 4d2253b
Should I avoid the code duplication? Is the alternative better, clear enough I mean: for (( i=1; i < ${#COMP_WORDS[@]} - 2; i++ )); do
case ${COMP_WORDS[i]} in
# Use $file as a name reference
-f|--file)
local -n file=hledgerFile ;;
--rules-file)
local -n file=hledgerRulesFile ;;
*)
continue ;;
esac
if [[ ${COMP_WORDS[i+1]} == '=' ]]; then
file=${COMP_WORDS[i+2]}
else
file=${COMP_WORDS[i+1]}
fi
# Pass it through compgen to unescape it
file=$(compgen -W "$file")
done Or, define a general function that can be reused to fetch any option argument from the command line: _hledger_optarg() {
local options=("$@")
local optarg i j offset
for (( i=1; i < ${#COMP_WORDS[@]} - 2; i++ )); do
offset=0
for j in "${!options[@]}"; do
if [[ ${COMP_WORDS[i]} == "${options[j]}" ]]; then
if [[ ${COMP_WORDS[i+1]} == '=' ]]; then
offset=2
else
offset=1
fi
# Pass it through compgen to unescape it
optarg=$(compgen -W "${COMP_WORDS[i + offset]}")
fi
done
((i += offset))
done
printf %s "$optarg"
}
_hledger() {
local hledgerArgs=("$@")
local hledgerFile
local hledgerRulesFile
hledgerFile=$(_hledger_optarg -f --file)
hledgerRulesFile=$(_hledger_optarg --rules-file)
[[ -f $hledgerFile ]] && hledgerArgs+=(--file "$hledgerFile")
[[ -f $hledgerRulesFile ]] && hledgerArgs+=(--rules-file "$hledgerRulesFile")
hledger "${hledgerArgs[@]}" 2>/dev/null
} |
Oops, is this a bug or a feature? If I pass two files to
If that is the expected behaviour I need to adjust the above to pass all files to the |
Thanks for this very comprehensive patch! It sounds good, I hope @schoettl will comment in a bit. Yes multiple input files are possible with multiple -f's. A single --rules-file overrides the rules for all csv files, so I think only the last --rules-file is significant. |
Thanks for mentioning me. I'll definitely review until weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I like your clean code and nice patches. I want to try it out also in the next days.
... but did I see it right in the code, that you still have context aware completions for e.g. I see your last point a bit differently. Sure, it's completion engine, not manual. But if the completions – at no costs – help the users to learn new features and remind them how these are spelled, I think this is a very good thing. Including single-letter like OK, but without tried it yet, if I understand your change in the m4 file correctly, you have considered this in the context-aware completion? |
Yep, that point in the first commit is no longer relevant because I've added them back in 6b35938. I thought as well that it doesn't cost us anything to offer them, so here they are again. But we don't need them in hledger/shell-completion/hledger-completion.bash Lines 224 to 259 in 6b35938
|
And thank you for the review @schoettl! |
And a hint for testing -- do not test in a shell which has sourced the original completion first, the environment will be compromised and some very fun things to debug may happen. |
Edit: Forget it, it doesn't take into account any input inserted by the user without the help of completion. Too good to be true! An idea: what if we take a more rigid stance and offer subcommand completion only as the second argument. We can get rid of that whole loop in the main function and reduce it to: if ((cword == 1)); then
_hledger_compreply "$(_hledger_compgen "$_hledger_complist_commands")"
return 0
else
subcommand=${words[1]}
subcommandOptions=_hledger_complist_options_${subcommand//-/_}
_hledger_compreply "$(_hledger_compgen "${!subcommandOptions}")"
fi And the whole function becomes: _hledger_completion_function() {
# Current treatment for special characters:
# - exclude colon (:) from COMP_WORDBREAKS
# - use comptop -o filenames to escape the rest
COMP_WORDBREAKS=${COMP_WORDBREAKS//:}
local cur prev words cword
_init_completion -n : || return 0
local subcommand subcommandOptions
if ((cword == 1)); then
_hledger_compreply "$(_hledger_compgen "$_hledger_complist_commands")"
return 0
else
subcommand=${words[1]}
subcommandOptions=_hledger_complist_options_${subcommand//-/_}
_hledger_compreply "$(_hledger_compgen "${!subcommandOptions}")"
fi
[[ $cur == -* ]] && return
# Most option arguments, query filters and accounts need to be escaped
# so it is easier to set this here and unset it only in specific cases
compopt -o filenames
# Option argument completion
_hledger_compreply_optarg && return
# Query completion
_hledger_compreply_query && return
# Subcommand specific
case $subcommand in
files|test) return 0 ;;
help)
compopt -o nosort +o filenames
_hledger_compreply_append "$(compgen -W "$(hledger help | tail -n 1)" -- "$cur")"
return 0
;;
esac
# Offer query filters and accounts for the rest
# Do not sort. Keep options, accounts and query filters grouped separately
compopt -o nosort -o nospace
_hledger_compreply_append "$(_hledger_compgen "$_hledger_complist_query_filters")"
if [[ -z $cur ]]; then
_hledger_compreply_append "$(_hledger_compgen "$(_hledger accounts --flat --depth 1)")"
else
_hledger_compreply_append "$(_hledger_compgen "$(_hledger accounts --flat)")"
fi
return 0
} Liberty traded for efficiency. Thoughts on this? |
Sorry I don't have more feedback.. I never use shell completions, because I'm usually in an emacs shell where they don't work (I believe). I must give the latest a try in a terminal.. When you two feel this is ready for merging, let me know. |
Hi @simonmichael, I would like some more time to optimize it as much as possible and to see the suggestions of @schoettl. No hurry. And I also use it mostly in Emacs with a ton of premade queries but now and then when you need to run that odd one... And with the vterm module for Emacs I've got a fully-fledged shell, yey! |
Cool, I will have to look into vterm.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a pretty ugly hack when it comes to Haskell but I do know my way around shell code, M4, GNU Make, and friends. I haven't compiled and given this a test drive but I did take a few minutes to read through the diff and the code looks great. Hats off to you for the attention to detail, very legible coding style, and proper handling of things people often get wrong in these contexts. I won't suggest it is perfect or bug free but what I did see looked very good and I'm looking forward to in landing. Thanks for taking the time to contribute to this project.
Thanks for the kind words @alerque. I am not a frequent contributor, and I am afraid I don't have the experience to do it properly. My excuses to @schoettl for piling up too many changes at once to review. I am open to critique so my next contribution won't suffer from my ignorance. Do give it a spin if you find the time though. I would like some more input on the ergonomics and of course... bugs! Cheers |
It might be a good idea to check the shell scripts in |
I have yet to look into Edit: And done... Nothing to worry about. Found a bug in |
fi | ||
compopt +o filenames | ||
_hledger_compreply "$(_hledger_compgen "$_hledger_complist_commands")" | ||
return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified even further like this:
subcommand=
break
...and sub-command completion will be offered next, but feels more vague, and means a few more instructions.
On extraction of commands, options, etc.It's all too easy to break a leg dancing with the regex. There should be a way to extract all commands, aliases, and the options they support using the |
@zhelezov yes, options/flags are grouped that way so that they can be reused consistently by hledger's various tools and commands. I believe https://hackage.haskell.org/package/cmdargs has something built in or an add-on that exposes the structure so you could consume it programmatically, eg there is https://hackage.haskell.org/package/cmdargs-browser that can generate a ui for any cmdargs-using program. This sort of thing will take some research. (If it were me, I would save that for a separate PR.) |
PS, I might be able to give more ideas on the hledger chat. |
@simonmichael Yeah, I will look into your suggestions on another rainy day and leave it for another PR. I will pop in #hledger to discuss this further then. @schoettl I was thinking that we should probably rename And a question -- looking at the Makefile, I see that And one more -- should we add an |
@alerque Thanks again for streamlining the build process. When you've got a minute I want to summon your make-fu again to check my last additions to the Makefile. |
Is there something similar to the BSD m4's |
Seems like great work ongoing here. When it comes to fancy make/m4, I'll just note our highest priority is robustness, portability and minimizing roadbumps and costs for installers/contributors/maintainers.
|
PS so that's a good question you're asking. Don't know the answer, sorry!
|
That's exactly my reason for asking because most of the included files are meant to be included literally, so it would be nice if there was a way to prevent accidental macro expansion or quoting issues, so future contributors aren't burdened with that. It's not that of an issue but something one has to keep in mind. |
Install the symlinks unconditionally. This way the user don't need to reinstall completion after adding an extension. Of course fine- grained control is possible with: `make install EXTENSIONS=web` e.g.
This reverts commit 0aeb88e.
Most of the included files are meant to be output literally, without any macro processing. Using `undivert` in place of `include` achieves that. This is a safety net against unsanitized input generated during the build. Also, developers editing the shell code stub shouldn't be constantly alert about triggering accidental macro expansion. In order that `undivert` mimics GNU behaviour on BSDs, the `-g` flag is used for the m4 invocation.
Just print a reminder to use `gmake` instead of spurting a bunch of errors.
Better be safe that sorry...
* `help` command's output is no longer listing help topics, so those completions are removed. * `check` command supersedes `check-dates`, `check-dupes`, etc.
4db9ac4
to
deb2a27
Compare
I see I can easily extract supported checks for completion with something like this: hledger check --help | sed -rn 's/^-\s+([a-z-]+)\s+-.*/\1/p' But I don't know how long that output will remain parsable, so tell me if you want that. Or simply hard-code them, as another possibility. Example patch: diff --git a/shell-completion/hledger-completion.bash.stub b/shell-completion/hledger-completion.bash.stub
index 2c4862615..08f490ba3 100644
--- a/shell-completion/hledger-completion.bash.stub
+++ b/shell-completion/hledger-completion.bash.stub
@@ -102,8 +102,18 @@ _hledger_completion() {
# Subcommand specific
case $subcommand in
+ check)
+ compopt +o filenames
+ _hledger_compreply "$(
+ _hledger_compgen "$(
+ hledger check --help |
+ sed -rn 's/^-[[:space:]]+([a-z][a-z-]+)[[:space:]]+-.*/\1/p'
+ )"
+ )"
+ return 0
+ ;;
# These do not expect or support any query arguments
- commodities|check|files|help|import|print-unique|test)
+ commodities|files|help|import|print-unique|test)
return 0
;;
esac Or rather do this at build time and include specific command arguments as heredocs, as we already do for options. |
About
Those are noops. |
Thanks @zhelezov. We should probably add
Good to know, thanks. |
I applied the hard-coding patch. However, I'm seeing an error when testing setup:
Probably because I'm on a mac. |
.SHELLSTATUS requires GNU Make 4.2+, while mac has only GNU Make 3.x. I commented out this error check as it didn't seem critical. I'm still not able to test this on mac though, see below.
|
Hi, I don't have access to a mac system and have not done any testing on one. bash v3.2 is ancient (released in 2006) but even so it should work. Are you sure you have Can you give this a try? First, make sure brew install bash-completion Then source it manually from, say if [ -f $(brew --prefix)/etc/bash_completion ]; then
. $(brew --prefix)/etc/bash_completion
fi |
Sorry @zhelezov, I'm not having any luck with this. I did
and restarted the shell, and to make sure, ran
Then I follow your install steps
and to make sure, ran
but still get
Maybe some other mac user can help figure this out. We do aim to be cross platform. |
Alright, tossing suggestions in the blind, but here are few things to try:
diff --git a/shell-completion/hledger-completion.bash.stub b/shell-completion/hledger-completion.bash.stub
index 2c4862615..2c0c26aad 100644
--- a/shell-completion/hledger-completion.bash.stub
+++ b/shell-completion/hledger-completion.bash.stub
@@ -29,7 +29,11 @@
_hledger_completion() {
local cur prev words cword
- _init_completion -n : || return 0
+ # _init_completion -n : || return 0
+ cword=$COMP_CWORD
+ words=("${COMP_WORDS[@]}")
+ prev=${words[cword-1]}
+ cur=${words[cword]}
# Current treatment for special characters:
# - exclude colon (:) from COMP_WORDBREAKS I need to think this through so use the patch only for testing. I have local uncommitted work that relies on functions provided by Edit: |
https://itnext.io/programmable-completion-for-bash-on-macos-f81a0103080b is a good doc. I think I'm still at the stage of getting bash completion libraries installed. I guessed this would get me running homebrew's bash 5.1, with bash-completion installed and _init_completion/__load_completion in scope, but not yet:
I guess we don't have many mac users trying this. I'll update when I find something new, and will merge in any case. |
Merged. Thanks @zhelezov for this epic update ! |
Success! With your patch. |
Wait @simonmichael, did you just merge a PR that is not working for you? BTW, a nice and comprehensive read the one you posted above. If I get it right, in order to make it work without patching on MacOS the user needs to upgrade
Followed by some manual work to whitelist the new version of bash in Or patch it in a way that degrades without fuss in case of missing/old/uninitialized diff --git a/shell-completion/hledger-completion.bash.stub b/shell-completion/hledger-completion.bash.stub
index 2c4862615..b91d53c47 100644
--- a/shell-completion/hledger-completion.bash.stub
+++ b/shell-completion/hledger-completion.bash.stub
@@ -29,7 +29,12 @@
_hledger_completion() {
local cur prev words cword
- _init_completion -n : || return 0
+ if ! _init_completion -n : 2>/dev/null; then
+ cword=$COMP_CWORD
+ words=("${COMP_WORDS[@]}")
+ prev=${words[cword-1]}
+ cur=${words[cword]}
+ fi
# Current treatment for special characters:
# - exclude colon (:) from COMP_WORDBREAKS Can you give this a try, with and without |
That's the stuff I tried. Unfortunately this is too much of a rabbit hole for me to spend more time on right now. I didn't want to release without this and PTA bash power users on mac seem to be a small niche. When someone figures it out though I bet our homebrew packagers will make it work. In fact we should ask them..
|
I see you added a new patch, I'll try it. I forgot, these lines are still a problem on mac. Even when I run make from within homebrew's bash 5.1. Perhaps make still uses the default system bash.
|
I jumped through the hoops again and all I got was this _init_completion error. Sorry, I'm sure this is awesome and I'd like to get on board with using shell completions but I don't have the patience to fiddle with it further. I reached out to one of our homebrew packagers, if they are interested we might get lucky. |
Alright then, leaving it as it is for the moment, I will address any issues with bugfix PRs later. As to |
This was supposed to be just a fix for #1404 but upon visiting the source
several issues became apparent and that is why the commit grew a bit more than
expected. A complete list of changes bellow:
Fix _HLEDGER_COMPLETION_TEMPDIR creates an additional directory on each shell invocation #1404
No more orphaned temporary directories. Commands, options, etc. that used to be
stored in there are included at build-time as here documents in the source.
Fix artifacts in
/tmp
after buildUpon fixing the above I became aware that the build itself was leaving behind a
heap of artifacts in
/tmp
that were not taken care of with amake clean
.Fixed by using temporary files and directories in the build directory. Makefile
and build scripts adjusted.
Produce command aliases
Regular expressions in build scripts changed to produce all command aliases
except single letter ones (see below)
Do not propose single letters completions
It is simply not useful and adds a lot of noise. It makes completion slower as
well because you need to hit yes on the prompt:
Display all 200 possibilities? (y or n)
output-options.sh
now excludes those.Query filters simplified
Keep only the prefix of the filter with the colon in
query-filters.txt
. Thischange has two reasons:
Fix completion impacts on global environment
The completion script was making a couple of changes to the global environment
which had an impact for the rest of the shell session.
set -o pipefail
: the change is hidden from the user and could lead to subtleerrors throughout the shell session
COMP_WORDBREAKS=" "
: this affects subsequent completions for us and otherprograms too. I exclude the colon
:
from its value and usecompopt -o filenames
to handle escaping of special characters for us. I wouldlike to find a solution without messing with
COMP_WORDBREAKS
but it is notstraight forward.
Fix hiding of legit subcommands
Completion was hiding all possibilities if a subcommand happens to be the prefix
of another. On typing
balance
, one should be proposedbalancesheet
andbalancesheetequity
as well.Return early
Try to complete depending on the current context and return immediately if
successful. Keep completion list relevant and as short as possible.
Context aware completion
_hledger_compreply_optarg()
_hledger_compreply_query()
--file
and--rules-file
arguments when proposing completions for theabove, see
_hledger()
Custom
compgen
wrappercompgen
is fairly complicated. There is no way to feed it a word list withliterals. It will mangle your input in so many ways that we cannot trust it. To
work around this several wrappers are used:
_hledger_compgen()
works with_hledger_quote_by_ref()
to process and escape newline separated input which isthen fed to
compgen
and finally inCOMPREPLY
through_hledger_compreply()
and
_hledger_compreply_append()
. It sounds messy and I guess it is, I would liketo find a more straight forward way to do it. I think it is still a way better
and safer interface with
readline
than trying togrep
our way through.Replace
declare
withlocal
Again, this script is sourced by the shell -- keep variable scopes as narrow as
possible. Oops, they are actually synonymous when used in a function but
local
declares our intentions explicitly.Use
compopt -o nosort
Often I resort to using it to keep different groups of completions together.
Whether this is more ergonomic or not is subjective. But our input lists are
already sorted at build-time so why not. Sort manually
query-filters.txt
whenediting it.
Remove irrelevant comments
And add some new ones :)
I think that is all. Give it a spin, try to abuse it, in and outside of quotes,
with some funky accounts, payees, tags, whatever, and tell me where it breaks or
behaves unexpectedly.