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

feat: support single-character flags #27

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

mmkal
Copy link
Sponsor

@mmkal mmkal commented Jun 18, 2024

Closes #26

Changes:

  • removed the check against 1-character flagNames
  • added a check against 1-character flags with an alias defined

Tests:

  • removed the test making sure 1-character flags were disallowed
  • added a test making sure 1-character flag names work
  • added a test making sure 1-character flag names can be used in alias groups along with longer flags (git commit -am hello)

Note:

I was surprised it was as easy as this. And from playing around it seems that type-flag doesn't seem to care if you use -- or -, for flags of any length. For example foo --message hello and foo -message hello both seem to work. Likewise foo -m hello and foo --m hello.

I wasn't sure if this was deliberate, I couldn't find it in the docs. But either way, I wanted to get this in along with test cases. So if the above is considered a bug, not regressing support for single-character flags would be part of the fix requirements if this goes in first.

Another note:

For this to make its way to cleye, there might be some special-casing needed for the --help renderer (to avoid suggesting --x 1 usage instead of -x 1 (although both work as noted above)). I can help with that too if you'd like, once this or something like it is published.

@privatenumber
Copy link
Owner

privatenumber commented Jun 20, 2024

So foo --message hello and foo -message hello have different behavior.

foo --message hello passes in 'hello' to message.

foo -message hello passes in 'hello' to the alias e, and true to messag (alias grouping). This is the same thing as git commit -am message where message is passed into m, and true to a. But it's just confusing because the aliases are grouped in a way to have some semantic meaning. There are tests that cover this behavior:

test('invalid consolidated aliases', () => {
const parsed = typeFlag(
{}, ['-invalidAlias'],
);
expect(parsed).toStrictEqual({
_: Object.assign([], { '--': [] }),
flags: {},
unknownFlags: {
i: [true, true, true],
n: [true],
v: [true],
a: [true, true],
l: [true, true],
d: [true],
A: [true],
s: [true],
},
});

I was thinking foo --m hello should not work because m is an alias... but this actually works in Git git commit --m 'hi' -a (where m is an alias) so maybe this is inadvertently expected behavior. I should add a test to cover this.

@privatenumber
Copy link
Owner

I think I was actually misinterpreting the Git behavior.

git --m message works only because there are no other flags for git commit that start with "m". But trying it with other flags raises errors:

$ git commit --a --m wip
error: ambiguous option: a (could be --allow-empty or --allow-empty-message)

I'll actually fix this first so it won't work.

@privatenumber privatenumber force-pushed the develop branch 2 times, most recently from 4d4ec18 to b581920 Compare June 20, 2024 05:17
@privatenumber
Copy link
Owner

I think this is basically ready to go.

Would you mind tackling the cleye update too?

You can install this version of type-flag with:

pnpm i 'privatenumber/type-flag#npm/single-character-flag'

@mmkal
Copy link
Sponsor Author

mmkal commented Jun 20, 2024

@privatenumber thanks for publishing! I tried to add to cleye but there's a build error:

image

I haven't had time to dive into it- the generics go very deep... but I suspect this error would appear from the latest develop branch too since types were not affected by this PR.

If you could publish a version based on latest develop I can test that theory?

@mmkal
Copy link
Sponsor Author

mmkal commented Jun 26, 2024

Ok, I was able to repro the above problem on the develop branch. I think type-flag no longer really works with cleye, as it stands. It is not related to this PR, so I'll turn this comment into an issue.

I used git bisect to find what change broke it (In case you want to try, I didn't do any fancy npm linking, I just copied the dist folder into my local cleye clone. So I just ran this command for each commit git bisect suggested: pnpm i; git checkout pnpm-lock.yaml; pnpm build; rm -rf ../cleye/node_modules/type-flag/dist; rsync -r dist ../cleye/node_modules/type-flag; (cd ../cleye; pnpm build 2> /dev/null). That command succeeded on master, and failed on develop because of the above typescript error.

Git bisect result:

5fcb9cc is the first bad commit
commit 5fcb9cc
Author: Hiroki Osame hiroki.osame@gmail.com
Date: Tue Apr 16 23:48:44 2024 +0900

style: remove any
path changes
README.md 6 +-
package.json 6 +-
pnpm-lock.yaml 2345 +++++++++++++++++++++++++++++++++++++------------------
src/get-flag.ts 2 +-
src/types.ts 9 +-
src/utils.ts 9 +-

6 files changed, 1587 insertions(+), 790 deletions(-)


I guess one of the any -> unknown changes broke it the typearg extends condition somehow. I'll try to find to time to dig into which exactly, but commenting in the meantime in case anything springs to mind.

@mmkal mmkal mentioned this pull request Jun 26, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support single-character flags
2 participants