-
Notifications
You must be signed in to change notification settings - Fork 20
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
impl(generator): better blockquote handling #346
impl(generator): better blockquote handling #346
Conversation
Rust treats any blockquote as a Rust code sample, unless it has the `norust` tag. Furthermore, `cargo test` will try to compile these code samples. This is all very good, but we generate the documentation comments from the Probobuf (or OpenAPI) comments, which may and do include blockquotes that are not Rust code. This change handles a few more blockquote types. We really need to parse the Markdown comments to handle all possible cases, but this can handle the most urgent cases.
/// ```norust | ||
/// DecimalString = | ||
/// [Sign] Significand [Exponent]; | ||
/// ``` |
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 think my hack is working correctly in this case, creating multiple blockquotes:
https://spec.commonmark.org/0.31.2/#example-242
However, this may not have been the intent upstream. We may want to fix the upstream code to use explicit >
markers.
@@ -763,11 +805,17 @@ impl Money { | |||
/// For instance, in Java this would be: | |||
/// | |||
/// com.google.type.PhoneNumber wireProto = | |||
/// com.google.type.PhoneNumber.newBuilder().build(); | |||
/// ```norust |
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 think my hack is not doing well in this case. Blockquotes require a blank line before they start and my hack does not require this. The code upstream is only indented 3 spaces, so about 1/2 of the lines appear to be blockquotes.
I am going to let it stand. The true fix is to use a Markdown parser, as suggested in #92.
@@ -437,41 +437,43 @@ impl Binding { | |||
/// | |||
/// Example Policy with multiple AuditConfigs: | |||
/// | |||
/// ```norust |
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.
These diffs are much easier to grok if you turn off whitespace differences.
/// HOME = 1; | ||
/// WORK = 2; | ||
/// } | ||
/// ``` |
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.
As noted elsewhere, ideally these two blockquotes would be merged, but the code (maybe by accident) is doing the right thing:
https://spec.commonmark.org/0.31.2/#example-242
The comments upstream (in the protos) need fixing.
Rust treats any blockquote as a Rust code sample, unless it has the
norust
tag. Furthermore,cargo test
will try to compile these codesamples. This is all very good, but we generate the documentation
comments from the Probobuf (or OpenAPI) comments, which may and do
include blockquotes that are not Rust code.
This change handles a few more blockquote types. We really need to parse
the Markdown comments to handle all possible cases, but this can handle
the most urgent cases.
Arguably this is partial progress on #92