-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
The default ShellCompDirective can be configured #2221
Open
albers
wants to merge
4
commits into
spf13:main
Choose a base branch
from
albers:default-shellcompdirective
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+74
−0
Open
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Loading status checks…
Apply suggestions from code review
Co-authored-by: ccoVeille <[email protected]>
There are no files selected for viewing
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to settle on what would be the best API for this new option.
Where do we expect the developer to set
CompletionOptions.DefaultShellCompDirective
?You've implemented option 2, and I wanted to ask if this was a conscious decision?
Whatever we settle on, I suggest making it clear on the comment above the definition of the new option, and also in the documenation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marckhouzam
It was a conscious decision.
Changing the default
ShellCompDirective
has massive side effects (effectively inverting the logic when and when not to write boilerplate handlers). I prefer an explicit opt-in for this change on the command level that does not propagate to child commands. This allows gradually introducing this new setting in just the places where it makes sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make a good point. But I can't help imagining a developer trying to use this option and being annoyed at having to set it on every single command; some programs have quite a lot of them; I feel like someone will come back and ask for another option to set a "global" default directive 😄 .
If we implement option 3 (affect command and its sub-commands), and a dev only wants to set the option for a single command, they could achieve this by setting the option on the specific command, and then setting the option to ShellCompDirectiveDefault on the first level children of that command. How awkward would that seem to you as an API?
I want to get this as best we can because once it is released, we will not be able to change it (except by introducing another completion option).
@thaJeztah Feel free to give your opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ccoVeille If you have an opinion, don't hesitate.
Or anyone else for that matter 😄
This is a community project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the root command should propagate it, but then:
if someone comes and say "hey, you should set this at root level dude", then some commands may be broken because they expect to have file completion, which is the default one, and it will be broken.
Most CLIs are never tested, first because it's not that easy, then because people are lazy or dislike tests.
So here choosing among the options is more about mitigating the consequences of each of them, and choose the one with the least consequences.
I prefer boilerplate and people having to add a directive on each command than having magical things that will hide problem, because things would be "magical"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For existing codebases, the second option seems to be most suitable for me because the others are too dangerous, or at least might violate the principle of least surprise.
For new projects, an approch with inheritance is probably a better choice. As a compromise, for those projects I would recommend the third option. It is also more flexible that option one, which assumes that all commands have the same file option density.
The choice depends on how much we weigh each of these use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering this new option will not have any impact unless a project explicitly decides to use it, aren’t we effectively dealing with the “new project” scenario all the time? I mean that if we document clearly that this option is recursive, when a project decides to use it, they will have to code it correctly.
If the option was active by default, then we would have to be more careful, but since that’s not the case I’m not worried about option 3; again, if it’s documented well.
@albers are you up for that or do you disagree? 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. I still think that cleaning up existing codebases is a relevant use case (I'm up to do this), but this use case definitely is a minor one. Let's look forward.
I will implement the recursive option 3.
Give me so me time for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marckhouzam I see a problem for the recursive implementation, perhaps you can help?
Imagine a subcommand resetting the custom
DefaultShellCompDirective
toShellCompDirectiveDefault
so that it does not get affected by the customDefaultShellCompDirective
of its parent.How can I distinguish between a
DefaultShellCompDirective
being0
because it was explicitly set and one the happens to be0
because this is its zero value?Only the second case should lead to parent traversal in search for a customized
DefaultShellCompDirective
.I was thinking of a test like this:
Is there a better way than introducing a setter for the
DefaultShellCompDirective
that also sets a boolean flag?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s ugly but the new
DefaultShellCompDirective
will need to be a pointer so we can tell between unset vs set to 0