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: adding interactive continue printer #384

Merged
merged 10 commits into from Sep 19, 2022

Conversation

luisdavim
Copy link
Contributor

Description

Adding a new interactive printer, it's very similar to the confirmation printer but allows for multiple options.

$ go run main.go                                           
Do you want to continue [Y/n/a/s]: 

 INFO  You answered: no

Scope

What is affected by this pull request?

  • Bug Fix
  • New Feature
  • Documentation
  • Other

To-Do Checklist

  • I tested my changes
  • I have commented every method that I created/changed
  • I updated the examples to fit with my changes
  • I have added tests for my newly created methods

@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #384 (0511586) into master (b319442) will increase coverage by 0.06%.
The diff coverage is 83.82%.

@@            Coverage Diff             @@
##           master     #384      +/-   ##
==========================================
+ Coverage   81.70%   81.77%   +0.06%     
==========================================
  Files          28       29       +1     
  Lines        2017     2085      +68     
==========================================
+ Hits         1648     1705      +57     
- Misses        357      365       +8     
- Partials       12       15       +3     
Impacted Files Coverage Δ
interactive_confirm_printer.go 91.93% <ø> (ø)
interactive_continue_printer.go 83.82% <83.82%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@luisdavim luisdavim force-pushed the interactive_continue_printer branch 9 times, most recently from b04ce9d to 757bc08 Compare July 26, 2022 16:47
@MarvinJWendt
Copy link
Member

MarvinJWendt commented Jul 26, 2022

Hi, sorry, we're lacking a bit of a contributing file. You don't have to edit any markdown or SVG files. If you add an example, you only need to commit the _examples/xxx/demo/main.go file. Everything else will be generated automatically when the PR is merged. You don't have to revert the files tho, they'll get overwritten by the CI.

I'll review the code later, thanks for the PR!

@luisdavim
Copy link
Contributor Author

luisdavim commented Jul 26, 2022

Hi, sorry, we're lacking a bit of a contributing file. You don't have to edit any markdown or SVG files. If you add an example, you only need to commit the _examples/xxx/demo/main.go file. Everything else will be generated automatically when the PR is merged. You don't have to revert the files tho, they'll get overwritten by the CI.

I'll review the code later, thanks for the PR!

oh, ok, do you want me to revert those changes? I generated them with go run ./ci but the SVG looks a bit off, it might be from my terminal size and screen resolution....

@MarvinJWendt
Copy link
Member

Hi @luisdavim, sorry for the delay, I am on vacation since the first August. I didn't have time to look into it before.

@luisdavim
Copy link
Contributor Author

No worries, enjoy your time off, this will be here waiting when you get back 😉

@MarvinJWendt
Copy link
Member

Hi @luisdavim, what do you think about adding this as a feature to the confirm printer? I don't really see the benefit in creating a new printer for it, as they would be pretty much the same.

@luisdavim
Copy link
Contributor Author

Sure, I can think about how that would look like.

@luisdavim
Copy link
Contributor Author

luisdavim commented Sep 11, 2022

Hi @MarvinJWendt, sorry for the delay on this. I was thinking about merging this and the confirm printers but I don't think there's a good way, we could replace the confirm printer with this one but that would be a breaking change, the confirm printer returns a bool and that only works if there are only 2 options. I do think there's room for both, the confirm printer covers the simpler use cases where only 2 options are needed and it is better in that situation because it returns a bool but there are many cases where something like yes/no/always/never are needed and that's where this printer comes in handy, before pterm had the interactive printers I was using https://github.com/AlecAivazis/survey and to cover this situation I had to use the select prompt with a custom filter, it works but it's not as clean, something like this:

func prompt(filename string) (string, error) {
	var response string
	if _, err := os.Stat(filename); err == nil {
		prompt := &survey.Select{
			Message: fmt.Sprintf("File %s exists. Overwrite?", filename),
			Options: []string{
				"diff: show a diff between the current file and the new file",
				"yes: overwrite the current file",
				"no: do not overwrite the current file",
				"all: overwrite all files",
				"None: do not overwrite any files",
				"quit: exit the program",
			},
			Filter: func(filterValue string, optValue string, optIndex int) bool {
				return strings.HasPrefix(optValue, filterValue)
			},
		}
		err = survey.AskOne(prompt, &response)
		return strings.ToLower(strings.Split(response, ":")[0]), err
	}
	return response, nil
}

@MarvinJWendt
Copy link
Member

MarvinJWendt commented Sep 12, 2022

No problem 😉
Yes, that makes sense to me.

I'll review the PR soon!

Copy link
Member

@MarvinJWendt MarvinJWendt left a comment

Choose a reason for hiding this comment

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

image

Hi, could you please make the prompt a little more descriptive? Currently, it's not very clear what the single letters do. Since this is a new printer, we could also accept a map, with the different options. For example map[string]string{"a": "yes to all", "y": "yes", ...}. A benefit of the map would be, that the keys would definitely be unique then. It would also make up a good reason for a new printer, since there would be a different output too (in comparison with the default confirm printer) 😄

I am thinking about something like:

USER DEFINED TEXT [Y (yes) / n (no) / a (all) / s (stop)]

You could also leave out the options ShowFullHandles and Handles then.

Signed-off-by: Luis Davim <luis.davim@sendoso.com>
@luisdavim
Copy link
Contributor Author

luisdavim commented Sep 15, 2022

image

Hi, could you please make the prompt a little more descriptive? Currently, it's not very clear what the single letters do. Since this is a new printer, we could also accept a map, with the different options. For example map[string]string{"a": "yes to all", "y": "yes", ...}. A benefit of the map would be, that the keys would definitely be unique then. It would also make up a good reason for a new printer, since there would be a different output too (in comparison with the default confirm printer) 😄

I am thinking about something like:

USER DEFINED TEXT [Y (yes) / n (no) / a (all) / s (stop)]

You could also leave out the options ShowFullHandles and Handles then.

Hi, the ShowFullHandles would get you:

USER DEFINED TEXT [Yes/no/all/stop]

I've updated the code to use a map as you suggested and made the prompt look like what you suggested:

USER DEFINED TEXT [Y (yes) / n (no) / a (all) / s (stop)]

And now I remember why I wasn't using a map, with a map we can't control the order and each time the prompt will be printed differently, I can avoid that by sorting the keys alphabetically but in some cases it would be nice to allow the user to specify the order.
I'll leave it with the map implementation for you to have a look at but if you want, I can try going back to the lists and instead of the boolean ShowFullHandles, I can add an enum that defines a prompt style, something like:

ShortHandles:

USER DEFINED TEXT [Y/n/a/s]

FullHandles:

USER DEFINED TEXT [Yes/no/all/stop]

HintHandles:

USER DEFINED TEXT [Y (Yes) / n (no) / a (all) / s (stop)]

Let me know what you think.

@MarvinJWendt
Copy link
Member

MarvinJWendt commented Sep 15, 2022

And now I remember why I wasn't using a map, with a map we can't control the order and each time the prompt will be printed differently, I can avoid that by sorting the keys alphabetically but in some cases it would be nice to allow the user to specify the order.

Yes, that makes sense. A string list is fine then.

To the ShowFullHandles, I think it would make sense to make that the default behavior and introduce ShortHandles / WithShortHandles. After looking at [Y (Yes) / n (no) / a (all) / s (stop)] long enough, it would probably make more sense to just stick to the Full/Short handles.

Thanks!

@MarvinJWendt MarvinJWendt self-assigned this Sep 15, 2022
@luisdavim
Copy link
Contributor Author

ok, so, should I revert my last commit and then make the full handles prompt the default?

@luisdavim
Copy link
Contributor Author

I've updated the PR, it now should the full handles by default and there's an option to display the short version as well.

@MarvinJWendt
Copy link
Member

Thanks! I'll review the code now :)

Copy link
Member

@MarvinJWendt MarvinJWendt left a comment

Choose a reason for hiding this comment

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

I have left some comments, some of them can be accepted in bulk directly on GitHub.

Could you also please append the selected value to the prompt, like in the confirm printer?

image

If you accept the suggestions from GitHub Web, please use the batch feature, otherwise each suggestion would be an own commit 😉
image


Thanks for the huge PR!

interactive_continue_printer.go Outdated Show resolved Hide resolved
interactive_continue_printer.go Outdated Show resolved Hide resolved
interactive_continue_printer.go Outdated Show resolved Hide resolved
interactive_continue_printer.go Outdated Show resolved Hide resolved
interactive_continue_printer.go Outdated Show resolved Hide resolved
interactive_continue_printer_test.go Outdated Show resolved Hide resolved
interactive_continue_printer_test.go Outdated Show resolved Hide resolved
interactive_continue_printer.go Outdated Show resolved Hide resolved
interactive_continue_printer.go Outdated Show resolved Hide resolved
interactive_continue_printer.go Outdated Show resolved Hide resolved
@luisdavim
Copy link
Contributor Author

I think I've addressed all your comments.

Copy link
Member

@MarvinJWendt MarvinJWendt left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution and your patience! 🚀
I'll have to update the docs website, I'll make the release then!

@MarvinJWendt MarvinJWendt merged commit 0ca1b0d into pterm:master Sep 19, 2022
@luisdavim luisdavim deleted the interactive_continue_printer branch September 19, 2022 10:39
Racer159 pushed a commit to defenseunicorns/zarf that referenced this pull request Sep 26, 2022
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/pterm/pterm](https://togithub.com/pterm/pterm) | require |
patch | `v0.12.46` -> `v0.12.47` |

---

### Release Notes

<details>
<summary>pterm/pterm</summary>

### [`v0.12.47`](https://togithub.com/pterm/pterm/releases/tag/v0.12.47)

[Compare
Source](https://togithub.com/pterm/pterm/compare/v0.12.46...v0.12.47)

<!-- Release notes generated using configuration in .github/release.yml
at master -->

#### What's Changed

##### Exciting New Features 🎉

- feat: adding interactive continue printer by
[@&#8203;luisdavim](https://togithub.com/luisdavim) in
[pterm/pterm#384

##### Other Changes

- bumped go.mod to 1.18 by
[@&#8203;MarvinJWendt](https://togithub.com/MarvinJWendt) in
[pterm/pterm#404

**Full Changelog**:
pterm/pterm@v0.12.46...v0.12.47

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click
this checkbox.

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/defenseunicorns/zarf).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMDIuNCIsInVwZGF0ZWRJblZlciI6IjMyLjIwMi40In0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Noxsios pushed a commit to defenseunicorns/zarf that referenced this pull request Mar 8, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/pterm/pterm](https://togithub.com/pterm/pterm) | require |
patch | `v0.12.46` -> `v0.12.47` |

---

### Release Notes

<details>
<summary>pterm/pterm</summary>

### [`v0.12.47`](https://togithub.com/pterm/pterm/releases/tag/v0.12.47)

[Compare
Source](https://togithub.com/pterm/pterm/compare/v0.12.46...v0.12.47)

<!-- Release notes generated using configuration in .github/release.yml
at master -->

#### What's Changed

##### Exciting New Features 🎉

- feat: adding interactive continue printer by
[@&#8203;luisdavim](https://togithub.com/luisdavim) in
[pterm/pterm#384

##### Other Changes

- bumped go.mod to 1.18 by
[@&#8203;MarvinJWendt](https://togithub.com/MarvinJWendt) in
[pterm/pterm#404

**Full Changelog**:
pterm/pterm@v0.12.46...v0.12.47

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click
this checkbox.

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/defenseunicorns/zarf).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMDIuNCIsInVwZGF0ZWRJblZlciI6IjMyLjIwMi40In0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants