Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

argparse: Make short flag names optional#7585

Merged
faho merged 8 commits into
fish-shell:masterfish-shell/fish-shell:masterfrom
faho:argparse-longonlyfaho/fish-shell:argparse-longonlyCopy head branch name to clipboard
Jan 1, 2021
Merged

argparse: Make short flag names optional#7585
faho merged 8 commits into
fish-shell:masterfish-shell/fish-shell:masterfrom
faho:argparse-longonlyfaho/fish-shell:argparse-longonlyCopy head branch name to clipboard

Conversation

@faho

@faho faho commented Dec 28, 2020

Copy link
Copy Markdown
Member

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.
@faho
faho force-pushed the argparse-longonly branch from 65f7ffe to 2867d64 Compare December 28, 2020 19:20

@zanchey zanchey left a comment

Copy link
Copy Markdown
Member

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!

Comment thread doc_src/cmds/argparse.rst Outdated
@zanchey

zanchey commented Dec 29, 2020

Copy link
Copy Markdown
Member

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

@faho

faho commented Dec 29, 2020

Copy link
Copy Markdown
Member Author

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?)

Comment thread doc_src/cmds/argparse.rst Outdated
Comment thread src/builtin_argparse.cpp Outdated
@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.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.