Skip to content

Conversation

@sandr01d
Copy link
Collaborator

@sandr01d sandr01d commented May 6, 2025

Check list

  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation

Description

Closes #435

Fish does not seem to be affected by this issue, so no changes necessary there.

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation change

Test environment

  • Shell
    • bash
    • zsh
    • fish
  • OS
    • Linux
    • Mac OS X
    • Windows
    • Others:

@sandr01d sandr01d self-assigned this May 6, 2025
@sandr01d sandr01d requested review from carlfriedrich, cjappl and wfxr May 6, 2025 18:03
Copy link
Collaborator

@carlfriedrich carlfriedrich left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @sandr01d!

I must confess, I actually don't like the \<command> syntax.

First (very subjective) argument is that I have never heard of this before. In contrast, using command as a prefix is a very common thing to do for me, even interactively, and in my perception this is the common way to do handle this (I might be wrong, though).

Second (kind of objective) argument is that people who don't know this syntax will have a hard time figuring out what it does, as this is difficult to google.

So I would still vote for prefixing everything with command.

A third approach (which I still like more than the backslash syntax) would be to call every command via its absolute path. The path could be found with which, so for example printf would become $(which printf).
This would not work for shell built-ins, though (set, unset, alias, export). However, I would consider it very unlikely that someone has aliased a shell built-in. We're not escaping if, then or while either, so I would vote against escaping built-ins in general.

Either way I would vote for adding a describing comment to the plugin on why we do this.

@sandr01d sandr01d force-pushed the prefix-commands branch 2 times, most recently from 9cc053a to a17bd3e Compare May 8, 2025 12:50
This prevents the wrong commands getting executed in case a user added a
shell alias with the same name.
@sandr01d sandr01d force-pushed the prefix-commands branch from a17bd3e to 96f65fc Compare May 8, 2025 12:56
@sandr01d
Copy link
Collaborator Author

sandr01d commented May 8, 2025

@carlfriedrich I've made changes accordingly. command does not support shell built-ins either, so I used builtin for those as pointed out in #435 (comment).

Copy link
Collaborator

@carlfriedrich carlfriedrich left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the update @sandr01d. I like this a lot more! LGTM.

@sandr01d sandr01d merged commit 38d74ba into wfxr:main May 18, 2025
3 checks passed
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.

Seemingly broken when grep is aliased for default options

3 participants