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: handling word answers as user inputs for install script #946

Merged
merged 3 commits into from
May 12, 2023

Conversation

SCedricThomas
Copy link
Contributor

@SCedricThomas SCedricThomas commented May 5, 2023

  • Add a changelog entry in the section "To Be Released" of CHANGELOG.md

fix #929

@SCedricThomas SCedricThomas self-assigned this May 5, 2023
@SCedricThomas SCedricThomas force-pushed the feat/929/handle-answers-with-multiple-letters branch from d60c2d2 to 656ed9b Compare May 5, 2023 12:35
@SCedricThomas SCedricThomas marked this pull request as ready for review May 5, 2023 12:36
@yohann-bacha yohann-bacha self-assigned this May 9, 2023
@@ -27,6 +27,18 @@ main() {
fi
}

ask() {
Copy link
Contributor

@yohann-bacha yohann-bacha May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Nice refacto 👍

CHANGELOG.md Outdated
@@ -8,6 +8,7 @@
* chore(deps): replace `github.com/ScaleFT/sshkeys` with `golang.org/x/crypto/ssh`
* fix(completion): fix zsh shebang reference
* feat(integration-link): add --follow arg to manual-deploy command
* feat(install.sh): handle answers with multiple letters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: i think the name of what we're doing is wrong. we're not handling "multiple letters", we become case insensitive

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are handling yes and no too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so what do you think about this sentence? I find it more accurate

Suggested change
* feat(install.sh): handle answers with multiple letters
* feat(install.sh): handling word answers as user inputs for install script

@yohann-bacha yohann-bacha changed the title Handle answers with multiple letters feat: case insensitive user input for install May 9, 2023
@yohann-bacha yohann-bacha changed the title feat: case insensitive user input for install feat: handling word answers as user inputs for install script May 9, 2023
Copy link
Member

@EtienneM EtienneM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a picky comment. Otherwise, if you tested it and it works, LGTM

dists/install.sh Outdated
Comment on lines 152 to 154
if ! ask "Do you want to replace it with version ${new_version}?" ; then
status "Aborting...\n"
exit -1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems we have a two spaces indentation in this file

Suggested change
if ! ask "Do you want to replace it with version ${new_version}?" ; then
status "Aborting...\n"
exit -1
if ! ask "Do you want to replace it with version ${new_version}?" ; then
status "Aborting...\n"
exit -1

@yohann-bacha yohann-bacha enabled auto-merge (squash) May 12, 2023 11:29
@yohann-bacha yohann-bacha merged commit 8626730 into master May 12, 2023
@yohann-bacha yohann-bacha deleted the feat/929/handle-answers-with-multiple-letters branch May 12, 2023 11:30
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.

[install.sh] Correctly handle answers with multiple letters
3 participants