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

argparse: Make short flag names optional #7585

Merged
merged 8 commits into from
Jan 1, 2021

Conversation

faho
Copy link
Member

@faho faho commented Dec 28, 2020

It was always a bit ridiculous that argparse required X-longflag if
that "X" was never actually used anywhere.

Since the short letter is for getopt's benefit, we can hack around
this with our old friend: Unicode Private Use Areas.

We have a counter, starting at 0xE000 and going to 0xF8FF, that counts
up for all options that don't have a short flag and provides one. This
gives us up to 6400 long-only options.

6.4K should be enough for everybody.

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

@faho faho added this to the fish 3.2.0 milestone Dec 28, 2020
It was always a bit ridiculous that argparse required X-longflag if
that "X" was never actually used anywhere.

Since the short letter is for getopt's benefit, we can hack around
this with our old friend: Unicode Private Use Areas.

We have a counter, starting at 0xE000 and going to 0xF8FF, that counts
up for all options that don't have a short flag and provides one. This
gives us up to 6400 long-only options.

6.4K should be enough for everybody.
Copy link
Member

@zanchey zanchey left a comment

Choose a reason for hiding this comment

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

Seems like a sensible thing to do!

doc_src/cmds/argparse.rst Outdated Show resolved Hide resolved
@zanchey
Copy link
Member

zanchey commented Dec 29, 2020

I couldn't see anything obvious in wgetopt, but is there any issue with this in LANG=C environments?

@faho
Copy link
Member Author

faho commented Dec 29, 2020

I couldn't see anything obvious in wgetopt, but is there any issue with this in LANG=C environments?

Tests show that it works (the test passes), and we only really need to represent some opaque values as wchar, so locale doesn't really apply - the width of a wchar is constant (and since we're under FFFF we should be okay with 2-byte wchar IIUC).

(technically we should check if any of the provided short flags is a PUA codepoint - or do we reject those on input anyway?)

doc_src/cmds/argparse.rst Outdated Show resolved Hide resolved
src/builtin_argparse.cpp Outdated Show resolved Hide resolved
@faho faho merged commit 7ea8e20 into fish-shell:master Jan 1, 2021
@faho faho deleted the argparse-longonly branch January 1, 2021 10:37
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 30, 2021
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

3 participants