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

completions/trash-cli: add completions for trash-cli #9560

Merged
merged 4 commits into from
Feb 13, 2023
Merged

completions/trash-cli: add completions for trash-cli #9560

merged 4 commits into from
Feb 13, 2023

Conversation

Jay-716
Copy link
Contributor

@Jay-716 Jay-716 commented Feb 9, 2023

Description

Add completions for trash-cli commands:
trash, trash-empty, trash-list, trash-put and trash-restore.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

Add completions for trash-cli commands:
trash, trash-empty, trash-list, trash-put and trash-restore.
@@ -0,0 +1,9 @@
complete -f -c trash-empty -s h -l help -d 'show help message'
complete -f -c trash-empty -l print-completion -a 'bash zsh tcsh' -d 'print shell completion script'
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to pass -x here, so that bash/zsh/tcsh are the only options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your swift review.
I'm sorry for making such a mistake and I have to add one more commit to fix that.
I realize that this pr may be abnormal and I can reopen one if you want to.
Thanks. :)

Pass ``-x`` to option ``--print-complete``
so that it will forcely require an argument.
Copy link
Contributor

@mqudsi mqudsi left a comment

Choose a reason for hiding this comment

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

I would request identification of the trash executable via some means as it isn't a unique name by any means and there are several implementations in different languages and for different platforms floating around.

CHANGELOG.rst Outdated
Comment on lines 44 to 48
- ``trash``
- ``trash-empty``
- ``trash-list``
- ``trash-put``
- ``trash-restore``
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you collapse these to one line ("trash and helper utilities trash-empty, trash-put, ...")?

@Jay-716
Copy link
Contributor Author

Jay-716 commented Feb 12, 2023

It's my fault to ignore other implementations of trash tools.
There are several things that need to be confirmed:

  • There are many trash tools, and which file should their completions locate? Can I merge all of the completions into a single file trash.fish or separate them into different .fish files and add implementation-specific completions when there are binaries with the same name?
  • May I collapse those lines in CHANGELOG.rst into ``trash`` and utilities of various implementations ?
  • May I squash all the commits into a single one before merging(if possible)?

And there are relatively popular trash cli tools I found:
https://github.com/andreafrancia/trash-cli
https://github.com/sindresorhus/trash-cli
https://github.com/ali-rantakari/trash
https://github.com/PhrozenByte/rmtrash

And as for the means to identify trash excutables, I think I can use some kind of trash --version | string match "xxx"
Would that be accetable? or any suggestions?
Sorry for my poor English. :)

@mqudsi
Copy link
Contributor

mqudsi commented Feb 12, 2023

No worries and your English is excellent!

To be clear, I'm not asking you to add completions for other versions/implementations of trash, but just to make sure that your completions only kick in for the one you intend them to. Matching against trash --version (usually not an approach we like because it can break in the future) is fine here.

You can squash the commits or I'll squash them when I'm merging. Whatever works for you.

If you are only adding this one implementation of trash, you can just use the changelog line I suggested. If you are going to go down the route of adding multiple versions of trash, you can use "trash (various implementations)" or similar.

To load multiple completions you can place completions for each "distribution" of trash in a function all in the same file then call the function depending on which "trash" you've identified as being installed. You don't need to do this for the specialized utilities like trash-empty, trash-put, etc unless those also collide (I imagine they don't).

…rash-cli

There are many different implementations of trash cli tools.
Use some means like help message to identify excutables of different implementations.
@Jay-716
Copy link
Contributor Author

Jay-716 commented Feb 13, 2023

Thanks for your patient guidance.
I decided to match the help message instead of the version because it may not be broken so easily.
And I just add the completions for only one implementation because I am not familiar with the others and I don't have an appropriate environment to test them, therefore I cannot ensure their quality. Sorry about that.

Comment on lines 17 to 19
if string match -qr "https://github.com/andreafrancia/trash-cli" (trash --help)
__trash_by_andreafrancia
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good - you're right, this is less likely to break than --version.

There's just one small problem: let's say tomorrow someone installs a different trash that doesn't have --help (so it'll print an error to stderr) or it writes --help output to stderr for some reason. In this case, trash.fish will print junk to the terminal because you are only capturing stdout in the subshell (trash --help).

Can you change this to either (trash --help 2>/dev/null) or (trash --help >&1) to prevent that from happening?

I can change it too but I wanted to share this with you as a learning opportunity.

Copy link
Contributor

@mqudsi mqudsi left a comment

Choose a reason for hiding this comment

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

Looks good except for the small issue with --help output that I commented on above.

``trash --help`` are used to identify the excutable in trash cli completion.
Some implementation may not have the ``--help`` option and therefore prints junks to stderr.
Redirect stderr to /dev/null.
@Jay-716
Copy link
Contributor Author

Jay-716 commented Feb 13, 2023

I should have taken the missing help option and stderr into consideration.
Thanks for your telling me about that and giving me this valuable learning opportunity.
It's so considerate and friendly of you haha. :)

@mqudsi mqudsi merged commit ce268b7 into fish-shell:master Feb 13, 2023
@zanchey zanchey added this to the fish 3.6.1 milestone Feb 22, 2023
zanchey pushed a commit that referenced this pull request Feb 27, 2023
Add completions for trash-cli commands:
trash, trash-empty, trash-list, trash-put and trash-restore.

``trash --help`` are used to identify the executable in trash cli completion.

(cherry picked from commit ce268b7)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants