feat(confirm): add --show-output#427
Merged
caarlos0 merged 2 commits intocharmbracelet:mainfrom Dec 12, 2024
Merged
Conversation
MikaelFangel
suggested changes
Oct 4, 2023
Contributor
MikaelFangel
left a comment
There was a problem hiding this comment.
I have added some suggestions to help fix your linting error.
Also, you can check if it passes linting before pushing by using golangci-lint run
Comment on lines
39
to
48
| } else { | ||
| os.Exit(1) | ||
| if o.ShowOutput { | ||
| confirmationText := m.(model).negative | ||
| if m.(model).confirmation { | ||
| confirmationText = m.(model).affirmative | ||
| } | ||
| fmt.Println(m.(model).prompt, confirmationText) | ||
| } | ||
|
|
||
| if m.(model).confirmation { | ||
| os.Exit(0) | ||
| } else { | ||
| os.Exit(1) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Here you should remove the else and outdent the code and also the last else statement can be outdented as well.
The code will look something like this.
}
if o.ShowOutput {
confirmationText := m.(model).negative
if m.(model).confirmation {
confirmationText = m.(model).affirmative
}
fmt.Println(m.(model).prompt, confirmationText)
}
if m.(model).confirmation {
os.Exit(0)
}
os.Exit(1)
Contributor
Author
There was a problem hiding this comment.
I must have missed the CI result notifications, really sorry.
This should be fixed as per you suggestions, thanks!
Regarding the model structure, do you think this commit is necessary?
Contributor
|
this looks good! can you fix the conflicts? if not, I can re-implement it on main... |
2bb47ec to
3d74294
Compare
Contributor
Author
|
No problem, it should be up to date now |
Contributor
|
thanks @vahnrr |
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
3 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.
Fixes nothing, this is a feature I found missing from a specific use case I had in mind.
Changes
Adds the
--show-outputflag togum confirmwhich prints the prompt and the chosen action to STDOUT.This merge adds 2 commits, only the first one (
4f9235b) is required for the feature to work.Since the feature doesn't affect
confirm/confirm.goI feel like the second (432182f) isn't necessary, but I still included it in case I misunderstood the purpose of the model structure.