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

Implement prompter package #131

Merged
merged 1 commit into from
Aug 25, 2023
Merged

Implement prompter package #131

merged 1 commit into from
Aug 25, 2023

Conversation

samcoe
Copy link
Contributor

@samcoe samcoe commented Aug 22, 2023

This PR implemented the prompter package which was mostly taken from gh with some small tweaks. Additionally, it includes a PrompterMock for use in tests.

cc cli/cli#7898

@samcoe samcoe self-assigned this Aug 22, 2023
@samcoe samcoe force-pushed the prompter branch 2 times, most recently from b6d6d78 to 72668d0 Compare August 23, 2023 18:43
@samcoe samcoe marked this pull request as ready for review August 23, 2023 18:43
@samcoe samcoe requested a review from andyfeller August 23, 2023 18:45
Copy link
Contributor

@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

Comparing these changes versus https://github.com/cli/cli/tree/trunk/internal/prompter, this is a welcome addition!

Minor nit but also a larger question regarding documentation. 🙇

Comment on lines 125 to 129
// LatinMatchingFilter returns whether the value matches the input filter.
// The strings are compared normalized in case.
// The filter's diactritics are kept as-is, but the value's are normalized,
// so that a missing diactritic in the filter still returns a result.
func latinMatchingFilter(filter, value string, index int) bool {
Copy link
Contributor

@andyfeller andyfeller Aug 23, 2023

Choose a reason for hiding this comment

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

(nit) Not sure if it matters but will match the function case just in case

Suggested change
// LatinMatchingFilter returns whether the value matches the input filter.
// The strings are compared normalized in case.
// The filter's diactritics are kept as-is, but the value's are normalized,
// so that a missing diactritic in the filter still returns a result.
func latinMatchingFilter(filter, value string, index int) bool {
// latinMatchingFilter returns whether the value matches the input filter.
// The strings are compared normalized in case.
// The filter's diactritics are kept as-is, but the value's are normalized,
// so that a missing diactritic in the filter still returns a result.
func latinMatchingFilter(filter, value string, index int) bool {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is something that I don't like about godoc comments... they encourage/enforce sentences but also the first letter can be uncapitalized, it bothers me.

@@ -84,3 +88,19 @@ func RelativeTimeAgo(a, b time.Time) string {

return fmtDuration(int(ago.Hours()/24/365), "year")
}

// RemoveDiacritics returns the input value without "diacritics", or accent marks.
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL

Comment on lines 1 to 53
package prompter

import (
"fmt"
"strings"
"testing"

"github.com/stretchr/testify/assert"
)

// PrompterMock provides stubbed out methods for prompting the user for
// use in tests. PrompterMock has a superset of the methods on Prompter
// so they both can satisfy the same interface.
type PrompterMock struct {
t *testing.T
selectStubs []selectStub
multiSelectStubs []multiSelectStub
inputStubs []inputStub
passwordStubs []passwordStub
confirmStubs []confirmStub
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming CLI extension integrators would use this in their own testing, how much documentation based on the cli/cli usage should be captured in documentation?

Do we expect people to depend upon https://pkg.go.dev/github.com/cli/go-gh or use markdown in the repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I wanted to write up an example that compiles like the ExamplePrompter() but it started getting complicated due to the fact that I was trying to write a test about writing a test. Perhaps just a normal comment is enough. I will think on it a bit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and added a test example in the comments.

@samcoe samcoe merged commit 421eac2 into trunk Aug 25, 2023
@samcoe samcoe deleted the prompter branch August 25, 2023 17:01
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.

2 participants