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

markdown: replace new line in usage #33

Merged
merged 2 commits into from Dec 27, 2022
Merged

Conversation

crazy-max
Copy link
Member

@@ -227,3 +228,7 @@ func mdCmdOutput(cmd *cobra.Command, old string) (string, error) {
func mdEscapePipe(s string) string {
return strings.ReplaceAll(s, `|`, `\|`)
}

func mdReplaceNewline(s string) string {
return regexp.MustCompile(`\r\n?|\n`).ReplaceAllString(s, "<br>")
Copy link
Member

Choose a reason for hiding this comment

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

Better to put the MustCompile either in a sync.Once or as a package var.

The regex probably should be making the \r optional, and not the \n (\r newlines are AFAIK only occurring on classic macOS);

Suggested change
return regexp.MustCompile(`\r\n?|\n`).ReplaceAllString(s, "<br>")
return regexp.MustCompile(`\r?\n`).ReplaceAllString(s, "<br>")

Copy link
Member

Choose a reason for hiding this comment

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

Oh; is XHTML still a thing nowadays? (I still have the tendency to use <br />, but perhaps we don't really care about that)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh; is XHTML still a thing nowadays? (I still have the tendency to use <br />, but perhaps we don't really care about that)

<br> is enough imo as markdown content is not served as application/xhtml+xml anyway (considered harmful). Also with HTML5 even if you write <br/> or <br /> it will eventually be converted to <br> by the browser.

Copy link
Member

Choose a reason for hiding this comment

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

considered harmful

Heh, yes, a lot of that was written when Internet Explorer was still "the largest browser", and, well, a lot of fun was needed in either case to write HTML that worked both in IE, and in browsers that were actually following standards 😂

But yeah, we don't do anything with XHTML(5), so 😂

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@codecov-commenter
Copy link

Codecov Report

Base: 64.36% // Head: 64.49% // Increases project coverage by +0.12% 🎉

Coverage data is based on head (6de4bc9) compared to base (77abede).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #33      +/-   ##
==========================================
+ Coverage   64.36%   64.49%   +0.12%     
==========================================
  Files           4        4              
  Lines         550      552       +2     
==========================================
+ Hits          354      356       +2     
  Misses        138      138              
  Partials       58       58              
Impacted Files Coverage Δ
clidocstool_md.go 76.50% <100.00%> (+0.28%) ⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@thaJeztah thaJeztah 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!

@thaJeztah thaJeztah merged commit 64ee549 into docker:main Dec 27, 2022
@crazy-max crazy-max deleted the usage-newline branch December 27, 2022 13:47
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.

None yet

3 participants