-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
uudoc, uucore_procs: move markdown parsing to HelpParser #4398
uudoc, uucore_procs: move markdown parsing to HelpParser #4398
Conversation
src/bin/uudoc.rs
Outdated
.get_long_about() | ||
.or_else(|| self.command.get_about()) | ||
{ | ||
if let Some(about) = self.command.get_about() { |
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'd rather keep this coming from the markdown directly. That enables us to use markdown syntax.
(Currently only backticks, which are removed before we pass them to clap, but render as inline code blocks in the online docs. Full compile-time markdown support is one of the things I'm working on in supporting for the new argument parser.)
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.
See this stub of a function:
coreutils/src/uucore_procs/src/lib.rs
Lines 52 to 54 in bc0273f
fn render_markdown(s: &str) -> String { | |
s.replace('`', "") | |
} |
Also I see we aren't using that function in about, so that's another thing I'll have to look into.
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 suppose we could skip it for now if it's half-baked anyway. For context, it's all part of this issue #3181, but I got a bit sidetracked on the argument parsing :)
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.
Hm, so the usage info should also come from the markdown instead of render_usage
?
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.
Yeah that one definitely, especially once your other PR for indentation gets merged: #4393. If it would come from render_usage
we'd get additional indentation in the online docs.
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.
No, there is already a trim
in the current implementation (https://github.com/uutils/coreutils/blob/main/src/bin/uudoc.rs#L179) and so #4393 doesn't affect the online docs.
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 right, that one doesn't matter then
GNU testsuite comparison:
|
d9ca2b5
to
574fdc9
Compare
574fdc9
to
cac0b0e
Compare
I moved the markdown parsing from |
22a4af6
to
0f5c37c
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.
I like the idea of putting this in a separate crate. I agree that the location doesn't feel quite right, but I can't think of a better one.
0f5c37c
to
16cee00
Compare
Initially I tried to add it to |
dc5fb2a
to
6265cf9
Compare
The most recent push resolves the merge conflict with #4569 |
GNU testsuite comparison:
|
sorry, it needs to be rebased :) |
6265cf9
to
9654b58
Compare
@cakebaker ping ? :) |
9654b58
to
6583231
Compare
GNU testsuite comparison:
|
This PR is related to #4393 (comment) and removes the check for the
about
section. It also removes the check for thelong_about
value because no tool sets this value.