-
Notifications
You must be signed in to change notification settings - Fork 915
Removed [A]ll - yes to all
#2849
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
Conversation
@lemonlambda thank you for raising this PR, and apologies for not coming back to you sooner about this. We have been discussing this PR internally, and we have been trying to decide the best path forward. When you said...
Can you provide any concrete examples for where this is being done, and what the output looks like? We would love to be able to compare what Chocolatey CLI is currently doing, to other package managers, but we wanted to avoid having to go hunting, if you had an example at hand. Apologies again about not coming back to you sooner here. As I have said a few times, "the wheels may turn slowly around here ...but they do turn" 😄 |
I can't show it from the terminal anymore so I've had to find it in code: https://github.com/Jguer/yay/blob/8ab365284651c99f598d6bc30868f30ec9488ce1/po/en.po#L40-L42
I understand :) |
Given the example above, I think we should use What are your thoughts? |
No objections from me. @lemonlambda as the original creator of this PR, would you have any objections to this compromise? |
I think it's shorter and more clean so sounds good to me. Although I still prefer just |
I have updated the branch to be up to date with the most recent commits, and did the |
After some discussion with the Chocolatey team, it has been decided that the prompt option for "All - yes to all" is rather verbose, and the need to include "yes to all" is unneeded, and not in line with how other package managers do it when prompted with similar choices. This commit removes the "All - yes to all" and replaces it with simply "All scripts", which is more succinct. In addition, the unit/pester tests have been updated to reflect this change. It was necessary to remove a unit test, since in the previous prompt "all - yes to all" was actually treated as two options, split on the "-", i.e. both "all" and "all - yes to all" were accepted answers to the prompt. This is no longer the case when the option is "All scripts", so only one unit test is now required.
@lemonlambda I have taken the liberty to rebase this PR and fix up the commit a little bit. I have fixed the unit tests, as there was a failing one due to the change made here, and I have updated the wording of the commit message to include information about what is being done here. I have taken this for a spin this morning and things are working as expected, with the correct prompt answers being handled: Once the CI builds complete, I will get this merged. Thanks again for your contribution to Chocolatey CLI! |
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.
LGTM!
Description Of Changes
Changed the prompt of
Do you want to run this script?([Y]es/[A]ll - yes to all/[N]o/[P]rint)
toDo you want to run this script?([Y]es/[A]ll/[N]o/[P]rint)
Motivation and Context
Unnecessary info as the command line app should assume the user knows what
[A]ll
, and this feature already exists for every other modern package manager on linux.Testing
No tests have been performed.
Change Types Made
Related Issue
Resolves #2848
Change Checklist