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

Change regex of abbr add to allow non-letters-only abbrs #2996

Merged
merged 1 commit into from May 6, 2016
Merged

Change regex of abbr add to allow non-letters-only abbrs #2996

merged 1 commit into from May 6, 2016

Conversation

etu
Copy link
Contributor

@etu etu commented May 6, 2016

Ran into this issue when I wanted to create abbrs with no letter characters.

@etu
Copy link
Contributor Author

etu commented May 6, 2016

This did work in 2.2, so it is a regression.

@faho faho merged commit 1c6f6df into fish-shell:master May 6, 2016
@faho
Copy link
Member

faho commented May 6, 2016

Merged, thanks!

Out of curiosity - what abbr did you try to create?

faho pushed a commit that referenced this pull request May 6, 2016
@faho faho added bug Something that's not working as intended regression labels May 6, 2016
@faho faho added this to the 2.3.0 milestone May 6, 2016
@faho
Copy link
Member

faho commented May 6, 2016

Also pushed to Integration_2.3.0, since it is a regression.

@@ -85,7 +85,7 @@ function abbr --description "Manage abbreviations"
set key $kv[1]
set value $kv[2]
# ensure the key contains at least one non-space character
if not string match -qr "\w" -- $key
if not string match -qr "[^\s]" -- $key
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't both of these incorrect? Shouldn't the key be a contiguous sequence of non-space characters? This allows "abc def" as a valid abbreviation. But if you actually execute abbr -a "abc def" "wtf" as soon as you type abc[space] it expands to def wtf. It seems to me the correct regex is '^\S+$'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, the regex is technically wrong but it doesn't matter because of how $mode_arg is constructed and the behavior of __fish_abbr_split. They collude to turn abbr -a "abc def" "wtf" into the equivalent of abbr -a abc "def wtf". Which is truly surprising behavior and clearly not what the user intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, sure, technically you're right. But with this change it's not a regression any more. So it's good to go for 2.3.0.

@etu
Copy link
Contributor Author

etu commented May 7, 2016

@faho I tried to make this:

function sudobangbang --on-event fish_postexec
    abbr !! sudo $argv[1]
end

I also added it to the wiki-page for "Bash Style History Substitution".

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants