Feature/better help output for negated options#104
Open
joaotavora wants to merge 2 commits intocacjs:mainfrom
Open
Feature/better help output for negated options#104joaotavora wants to merge 2 commits intocacjs:mainfrom
joaotavora wants to merge 2 commits intocacjs:mainfrom
Conversation
Probably doesn't make any practical difference, since this is only used for the Command array, but less lines of code :-) * src/utils.ts (findLongest)
Not only doesn't show the `(default: true)` for negated options, which
was quite confusing, but merges them with the line describing the
affirmative case.
Added a new test. Since it relies on help output, it is a bit
brittle, like the other "snapshot"-based ones. I didn't the same
execa snapshot for this because it would make a test take a lot longer
to run (becasue spawns a process).
* README.md (Options): Mention [no-]foo syntax.
* src/Command.ts (outputHelp): Consider options with negated
counterparts when printing.
* src/__test__/index.test.ts ('negated option help output'): New test.
egoist
reviewed
Feb 21, 2021
| if (o.negated) return 0 | ||
| const l = o.rawName.length | ||
| // '5' is the length of '[no-]'. As to the 1, I have no clue... | ||
| return negatedOpts[o.name] ? l + 5 + 1 : l |
Author
There was a problem hiding this comment.
It's needed, but I don't know why :-D Try taking it out. It didn't work when I tested...
Author
There was a problem hiding this comment.
If you take out the 1, not sure if the unit test will show the bug, but if you hand-test with some affirmative and negated options you'll see it, I think.
I understand that adding a "mysterious 1" to your codebase is silly. If you want I'll investigate the real reason, and type that into the comment.
Collaborator
There was a problem hiding this comment.
yeah, a failing test would be nice.
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits here. The first is completely orthogonal to this, you can skip it, but I couldn't resist.
The second one implements the idea briefly discussed. I added a test. See the commit messages for details.
By the way, you have some very intrusive git hooks setup. Whenever I do a commit, it takes 7 seconds to run the tests. Plus, something in the hooks runs a some kind of process behind the scenes that substantially reformat the whole file, changing my commit, including parts of the file I didn't even touch. I do not want to author those formatting changes (at least not as a part of this PR). It's totally fine to want a certain formatting, but IMO it's a bad idea to attribute those to people wanting to add specific features or fix specific bugs. Just my opinion.
Even doing a rebase to get rid of them was complicated, I had to completely wipe out
.git/hooksto be able to function properly.Other than that, fantastic nifty little project you have here, congratulations!