todotxt / todo.txt-cli

☑️ A simple and extensible shell script for managing your todo.txt file.
http://todotxt.org
GNU General Public License v3.0
5.56k stars 713 forks source link

Add initial fish subcommand completions #341

Closed asfaltboy closed 3 years ago

asfaltboy commented 3 years ago

Before submitting a pull request, please make sure the following is done:

Description

This adds fish shell command completions as mentioned in #142 ; this is by no means complete but it's a good start for new users, like myself, who can't remember what's what yet.

Dear reviewer, please try out "fish shell" copying this file into ~/.config/fish/completions/ should enable the completions. The parsing of options' help might be overkill if the command don't change very often. Please add any other ideas or suggestions for this initial MVP.

Implemented

Todo

References

inkarkat commented 3 years ago

Looks like a welcome addition for Fish users. (I'm not sure how many that would be, though.)

I wonder whether it wouldn't be better to implement the completion in terms of the existing Bash completion, if necessary through extracting some common generic functions. This way, the nice set of tests of Bash completion (10 files so far) could be leveraged, and maybe even all of the functionality could be offered for Fish as well.

The lack of test coverage worries me the most; the existing developers will probably not be well versed in Fish syntax, and unless you come back and complete the implementation soon, there will be requests and maybe even patches, and then who's gonna decide whether to accept it (without test coverage)?

I know that it's a lot more effort and challenge to come up with tests, especially for something like completion that is quite integrated into the shell itself. Maybe it would be better to keep this as a separate project so far; we could add a link here (Fish shell users refer to asfaltboy/todo.txt-cli-fish-completion). It could still be integrated here later, once test coverage has been added or the project has matured so much and received so many stars that it could be deemed "stable and practically tested".

I hope I haven't put you off as being too hesitant and negative; I just remember when I submitted my first changes to todo.txt, and Gina challenged me as well to add test coverage, and it helped both me as a developer and also the project's quality in the end.

inkarkat commented 3 years ago

Regarding the functionality, I wonder why you're restricting +project completion just to add[m] actions, as projects make sense with several other actions, too. The approach of the Bash completion is to rather offer too much than too little.

Adding completion for @context is analog to +project, so it would be a simple matter of copy-and-paste to add that, too.

karbassi commented 3 years ago

I'm going to keep this open for a month to give @asfaltboy time to bring the PR to a mergeable state.

@asfaltboy We'd love to merge this PR, but as @inkarkat noted, it's incomplete at this point. Could you finish the TODOs and write some tests in the next month?

asfaltboy commented 3 years ago

I could not find any concensus around how to test fish completions, so I don't mind scraping it and maybe linking to it in some doc/README/FAQ in case other people ask around