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

Refactored question defaults #7

Merged
merged 5 commits into from
Jun 29, 2022
Merged

Refactored question defaults #7

merged 5 commits into from
Jun 29, 2022

Conversation

GavinBurris42
Copy link
Owner

…ults if available

Issue #, if available: awslabs#75

Description of changes: Refactored question defaults to set saved user configurations as defaults if available

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@GavinBurris42 GavinBurris42 requested a review from snay2 June 23, 2022 21:08
Copy link

@cjerad cjerad left a comment

Choose a reason for hiding this comment

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

A good start. Just some questions about handling nil and errors, and some feedback to simplify logic and avoid anti-patterns.

cmd/launch.go Outdated Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
cmd/launch.go Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
cmd/launch.go Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
cmd/launch.go Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
pkg/question/question.go Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
cmd/launch.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
pkg/question/question.go Outdated Show resolved Hide resolved
Copy link

@cjerad cjerad left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Copy link
Collaborator

@snay2 snay2 left a comment

Choose a reason for hiding this comment

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

Confirmed with Gavin offline that the unit tests are passing, even though this PR didn't run them automatically.

The existing unit tests now verify that the original code paths still function the same when user defaults are not supplied. I would also like to see some new tests to positively verify that user defaults are chosen instead when passed in.

But the new tests can be in a separate PR. This one looks good to me now.

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.

3 participants