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

string: Allow collect --allow-empty to avoid empty ellision #8054

Merged
merged 2 commits into from
Jul 9, 2021

Conversation

faho
Copy link
Member

@faho faho commented Jun 11, 2021

Currently we still have that issue where

test -n (thing | string collect)

can return true if thing doesn't print anything, because the
collected argument will still be removed.

So, what we do is allow --no-empty --allow-empty to be used, in which case we
print one empty argument.

This means

test -n (thing | string collect -a)

can now be safely used.

"noallow-empty" isn't the best name for this flag, but string's design
really incentivizes reusing names, and it's not terrible.

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

Currently we still have that issue where

    test -n (thing | string collect)

can return true if `thing` doesn't print anything, because the
collected argument will still be removed.

So, what we do is allow `--no-empty` to be used, in which case we
print one empty argument.

This means

    test -n (thing | string collect -n)

can now be safely used.

"no-empty" isn't the best name for this flag, but string's design
really incentivizes reusing names, and it's not *terrible*.
@faho faho added this to the fish-future milestone Jun 11, 2021
`--no-empty` does the exact opposite for `string split` and split0.

Since `-a`/`--allow-empty` already exists, use it.
@faho
Copy link
Member Author

faho commented Jun 11, 2021

"no-empty" isn't the best name for this flag, but string's design
really incentivizes reusing names, and it's not terrible.

I have now switched to using "--allow-empty" - because "--no-empty" does the opposite in split/split0 (it causes them to remove empty arguments)

@faho faho changed the title string: Allow collect --no-empty to avoid empty ellision string: Allow collect --allow-empty to avoid empty ellision Jun 11, 2021
@ridiculousfish
Copy link
Member

An alternative to this is to support quoted substitutions, old #159:

test -n "$(thing | string collect)"

the advantage here is it works for all commands, not just string. But I don't mind --no-empty

@faho
Copy link
Member Author

faho commented Jun 12, 2021

An alternative to this is to support quoted substitutions, old #159:

Oh, sure. And that's something I'd eventually like to have. But I've looked into it, and it's not trivial to add.

While this exists now.

@ridiculousfish
Copy link
Member

Ok, seems reasonable; go for it.

@faho faho merged commit 0e1f510 into fish-shell:master Jul 9, 2021
@faho faho modified the milestones: fish-future, fish 3.4.0 Jul 9, 2021
@faho faho deleted the collect-nonempty branch July 9, 2021 19:21
@fish-shell fish-shell deleted a comment Mar 14, 2022
@fish-shell fish-shell locked as spam and limited conversation to collaborators Mar 14, 2022
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.

None yet

2 participants