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

fish_indent: Remove useless quotes #6722

Closed
wants to merge 6 commits into from

Conversation

faho
Copy link
Member

@faho faho commented Mar 8, 2020

Description

This changes fish_indent so that it strips quotes where they don't have any use.

In particular what it will do is turn this

set --local composer_cmds 'about' 'archive' 'browse' 'check-platform-reqs' 'clear-cache' 'config'

into this:

set --local composer_cmds about archive browse 'check-platform-reqs' 'clear-cache' config

As you can see it currently doesn't handle everything but instead tries unquoting and if that only leaves alphanumeric characters it leaves the quotes off.

(I initially tried a complicated scheme in which I'd try unescaping twice to see if anything changes, but that had issues, in particular with chars like ^ or % that might only be special some of the time).

TODOs:

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

@faho faho added this to the fish 3.2.0 milestone Mar 8, 2020
Copy link
Member

@krobelus krobelus left a comment

Choose a reason for hiding this comment

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

This makes scripts less confusing to read, I like it.

src/fish_indent.cpp Outdated Show resolved Hide resolved
src/common.h Outdated Show resolved Hide resolved
// If no non-alphanumeric char is left, use the unescaped version.
// This can be extended to other characters, but giving the precise list is tough,
// can change over time (see "^", "%" and "?", in some cases "{}") and it just makes people feel more at ease.
if (std::find_if_not(unescaped.begin(), unescaped.end(), fish_iswalnum) == unescaped.end() && !unescaped.empty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add just _ and - to the allowed characters? Since they are commonly used for compound names.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like it if we could anchor this to some other character set. I don't think we have a set of "safe" characters anywhere. We could also pick valid variable names, which is this plus "_", but not "-".

Note that this would have real impact on how our completions format - there are some that have "kebab-case" commands.

Or we just declare a random set of valid characters, that wouldn't be too awful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I pushed an additional list of "-", "_" and "/". To show the difference I added another commit just with those changes, but I'll remove that when merging.

src/fish_indent.cpp Outdated Show resolved Hide resolved
faho added 6 commits March 8, 2020 20:26
This tries to see if quotes guard some expansion from happening. If it
detects a "weird" character it'll leave the quotes in place, even in
some cases where it might not trigger.

So

    for i in 'c' 'color'

turns into

    for i in c color

The rationale here is that these quotes are useless, wasting
space (and line length), but more importantly that they are
superstitions. They don't do anything, but look like they do.

The counter argument is that they can be kept in case of later
changes, or that they make the intent clear - "this is supposed to be
a string we pass".
This is sometimes not wanted, like in the case of fish_indent.
This adds "_", "-" and "/" as characters for words we strip quotes
from.

The list is admittedly a tad arbitrary.
@faho
Copy link
Member Author

faho commented Mar 9, 2020

Merged as b25f72f..9f984ee after re-running the indent (which also gets around merge conflicts) and making one teensy change to a check.

@faho faho closed this Mar 9, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2020
@faho faho deleted the indent-useless-quotes branch September 23, 2021 09:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants