-
-
Notifications
You must be signed in to change notification settings - Fork 190
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
add prefix for enum type #481
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I had completely missed the existence of this Pull Request 🙇
d20ebb1
to
4620275
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick response!
I left a comment, so could you take it when you have time?
4620275
to
883102f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Most changes are the thing I expected.
I left a nit comment about prefix string, could you take a look?
choices := make([]string, 0, len(e.GetValues())) | ||
for _, v := range e.GetValues() { | ||
choices = append(choices, v.GetName()) | ||
} | ||
|
||
choice, err := r.selectChoices(e.GetFullyQualifiedName(), choices) | ||
choice, err := r.selectChoices(prefix, choices) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the prompt has an unnecessary character :
, it's better to trim it like ? choice (TYPE_ENUM) =>
.
This can be pass a template through Templates.Label
of promptui.Select
(the implementation of the prompt).
FYI: the default template label is maybe here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the diff. Wait a minute, maybe it's better to pass the template.
Codecov Report
@@ Coverage Diff @@
## master #481 +/- ##
==========================================
- Coverage 80.23% 80.15% -0.09%
==========================================
Files 57 57
Lines 3840 3844 +4
==========================================
Hits 3081 3081
- Misses 532 536 +4
Partials 227 227 |
883102f
to
0fc2236
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for your contribution 🍻
For multiple fields of the same enum type in a proto message, it's confusing not to prompt which field to enter...
After the change: