Add command validation #8
Conversation
Deprecate the Example Title and Postamble. It ended up not really being used and makes formating simpler in the case where some examples have titles and others do not. Add validation to enforce that the preamble starts with a capital and ends with a colon. Update all commands to follow this format.
There was a problem hiding this comment.
Looks good to me.
⌛ I wonder if it would be helpful to collect all errors instead of returning only the first. It could make it a bit easier to fix all problems in one swoop instead of having to rerun the tests multiple times.
I am using Feedback Ladder annotations to make the importance/urgency of comments easier to understand.
| Emoji | Name | Importance/Urgency |
|---|---|---|
| 🏔 | Mountain | This change should not go live. Back to the drawing board. |
| 🧗🏻♂️ | Boulder | Has to be addressed before this can be merged. |
| ⚪️ | Pebble | Needs to be addressed. Can be in a future PR. |
| ⏳ | Sand | Needs to be considered. Can be in a future PR. |
| ✏️ | Pen | Typo or grammatical error. Can be addressed in a future PR. |
| 💨 | Dust | "Take it or Leave it" |
|
|
||
| // Check for duplicates | ||
| if _, ok := aliases[alias]; ok { | ||
| return fmt.Errorf("duplicate alias %q found", alias) |
There was a problem hiding this comment.
💨 The error messages switch between stating what is wrong "duplicate alias %q found" and stating what is allowed/not allowed "command name cannot be an alias". I think it would be good to be consistent and always state what is not allowed "alias %q: aliases can not be duplicated" (or "alias can not be empty" below).
| } | ||
|
|
||
| // Validate that the help text is set | ||
| if c.ShortHelp == "" || c.LongHelp == "" { |
There was a problem hiding this comment.
💨 It could be helpful to have these as two checks with different error messages.
| if e.Command == "" { | ||
| return fmt.Errorf("command cannot be empty") | ||
| } else if !strings.HasPrefix(e.Command, "$ ") { | ||
| return fmt.Errorf("example command must start with $") | ||
| } |
There was a problem hiding this comment.
⌛ Should we validate that newlines are prefixed with a \?
This PR adds command validation to codify many formatting guidelines. It also deprecates unused fields in command Examples.
Moving forward command validation will be run as a test to avoid non-conventional commands from being added.
Would suggest reviewing the PR commit by commit.