-
Notifications
You must be signed in to change notification settings - Fork 24
docs: surround remediation flags with backticks for prompted feedback #135
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
==========================================
+ Coverage 63.46% 63.49% +0.02%
==========================================
Files 212 212
Lines 22345 22345
==========================================
+ Hits 14182 14188 +6
+ Misses 7080 7078 -2
+ Partials 1083 1079 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mwbrooks
left a comment
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.
✅ Sounds good! We may have similar formatting issues in other error messages as well. 🤔 I left a suggestion that we may want to consider, since <arg> is common usage in CLI help messages and back ticks can be a dangerous character when copy & pasted in the terminal.
| Message: "The name of the feedback is required", | ||
| Remediation: strings.Join([]string{ | ||
| "Please provide a --name <string> flag or remove the --no-prompt flag", | ||
| "Please provide a `--name <string>` flag or remove the `--no-prompt` flag", |
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.
note: It's a new dimension for us to write error messages with markdown docs rendering in mind. We'll also want to ensure this always looks good on the terminal. Back ticks can be a dangerous character in the terminal when copy & pasted, but I agree that it reads well.
question: We may be able to update our slack docs command to render instead of < rather than adding backticks to our error messages.
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.
oh i just went ahead and added the backticks because i saw them in other errors -- if it creates issues i can add an exception on our docs-cleaning scripts instead. i was trying to avoid adding hardcoded examples in that for maintenance reasons, but if it actually affects terminal output possibilities then it seems like the better option!
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.
👋 Adding to @mwbrooks suggestion - I lean towards improving docgen outputs with expected escaping for these cases!
But I do think this change makes the remediation more clear, FWIW.
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.
@lukegalbraithrussell Totally cool! I think we should merge this PR because it immediately fixes the docs experience. We can create a task to improve the docgen command if we want to avoid adding back ticks to every occurrence of < and >. ![]()
zimeg
left a comment
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.
@lukegalbraithrussell LGTM! Thanks for finding and following up with this fix! 📚 ✨
| Message: "The name of the feedback is required", | ||
| Remediation: strings.Join([]string{ | ||
| "Please provide a --name <string> flag or remove the --no-prompt flag", | ||
| "Please provide a `--name <string>` flag or remove the `--no-prompt` flag", |
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.
👋 Adding to @mwbrooks suggestion - I lean towards improving docgen outputs with expected escaping for these cases!
But I do think this change makes the remediation more clear, FWIW.
Summary
This error has unescaped
<in its description which our docs sites does not like.Requirements