Skip to content

Refactor Markdown length-limited summary implementation - #88173

Merged
bors merged 7 commits into
rust-lang:masterfrom
camelid:refactor-markdown-length-limit
Aug 29, 2021
Merged

Refactor Markdown length-limited summary implementation#88173
bors merged 7 commits into
rust-lang:masterfrom
camelid:refactor-markdown-length-limit

Conversation

@camelid

@camelidcamelid commented Aug 19, 2021

Copy link
Copy Markdown
Member

This PR is a new approach to #79749.

This PR refactors the implementation of markdown_summary_with_limit(),
separating the logic of determining when the limit has been reached from
the actual rendering process.

The main advantage of the new approach is that it guarantees that all
HTML tags are closed, whereas the previous implementation could generate
tags that were never closed. It also ensures that no empty tags are
generated (e.g., <em></em>).

The new implementation consists of a general-purpose struct
HtmlWithLimit that manages the length-limiting logic and a function
markdown_summary_with_limit() that renders Markdown to HTML using the
struct.

r? @GuillaumeGomez

@camelidcamelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 19, 2021
@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2021
@camelid
camelidforce-pushed the refactor-markdown-length-limit branch from c42090c to 37a800cCompareAugust 19, 2021 22:34
camelid
camelid commented Aug 19, 2021
View reviewed changes
Comment threadsrc/librustdoc/html/markdown.rs Outdated
Comment threadsrc/librustdoc/html/length_limit.rs Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Refactor Markdown length-limited summary implementation
39ef8ea
This commit refactors the implementation of
`markdown_summary_with_limit()`, separating the logic of determining
when the limit has been reached from the actual rendering process.
The main advantage of the new approach is that it guarantees that all
HTML tags are closed, whereas the previous implementation could generate
tags that were never closed. It also ensures that no empty tags are
generated (e.g., `<em></em>`).
The new implementation consists of a general-purpose struct
`HtmlWithLimit` that manages the length-limiting logic and a function
`markdown_summary_with_limit()` that renders Markdown to HTML using the
struct.
@camelid
camelidforce-pushed the refactor-markdown-length-limit branch from 37a800c to 39ef8eaCompareAugust 19, 2021 23:23
GuillaumeGomez
GuillaumeGomez reviewed Aug 20, 2021
View reviewed changes
Comment threadsrc/librustdoc/html/length_limit.rs Outdated
GuillaumeGomez
GuillaumeGomez reviewed Aug 20, 2021
View reviewed changes
Comment threadsrc/librustdoc/html/length_limit.rs Outdated
@GuillaumeGomez

GuillaumeGomez commented Aug 20, 2021

Copy link
Copy Markdown
Member

Overall, looks pretty nice. Please add tests to ensure it works as expected. Also, not sure to see the added value of ControlFlow in here...

Last point: we used to create the string with a capacity whereas you don't anymore.

@camelid

camelid commented Aug 20, 2021

Copy link
Copy Markdown
MemberAuthor

Please add tests to ensure it works as expected.

Will do. 👍

Also, not sure to see the added value of ControlFlow in here...

The value is that it allows automatically stopping the iteration, and it communicates the intent better than Result (because it's not an error if it stops early) and better than bool (because bool doesn't work with try_for_each, and because it can be unclear whether values mean "stop" or "continue").

Or are you referring to the feature gate? Note that ControlFlow itself has recently been stabilized, and that feature gate is just for some convenience API like ControlFlow::BREAK = ControlFlow::Break(()).

Last point: we used to create the string with a capacity whereas you don't anymore.

See #88173 (comment).

@GuillaumeGomez

GuillaumeGomez commented Aug 20, 2021

Copy link
Copy Markdown
Member

The value is that it allows automatically stopping the iteration, and it communicates the intent better than Result (because it's not an error if it stops early) and better than bool (because bool doesn't work with try_for_each, and because it can be unclear whether values mean "stop" or "continue").

Or are you referring to the feature gate? Note that ControlFlow itself has recently been stabilized, and that feature gate is just for some convenience API like ControlFlow::BREAK = ControlFlow::Break(()).

No, I meant ControlFlow in general. But it's mostly a nit at this point.

@camelidcamelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2021
@camelid
Clarify wording in docs
09f0876
@camelid
Use the length limit as the initial capacity
74147b8
The length limit turns out to be a surprisingly good heuristic for
initial allocation size. See here for more details [1].
[1]: rust-lang#88173 (comment)
@camelid
Use write!
d18936a
@camelidcamelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 22, 2021
camelid
camelid commented Aug 22, 2021
View reviewed changes
Comment threadsrc/librustdoc/html/length_limit.rs Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
camelidforce-pushed the refactor-markdown-length-limit branch from 988c49b to c4a9b75CompareAugust 22, 2021 01:38
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelidcamelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 22, 2021
@camelid
camelidforce-pushed the refactor-markdown-length-limit branch from c4a9b75 to 4a75607CompareAugust 25, 2021 20:45
@camelidcamelid removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 25, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
camelidforce-pushed the refactor-markdown-length-limit branch from b5a082d to 84fd81bCompareAugust 26, 2021 00:43
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Add tests for HtmlWithLimit
f8ca576
@camelid
Assert that tag_name is alphabetic
d932e62
@camelid
Don't panic if close_tag() is called without tags to close
4478ecc
This can happen when a tag is opened after the length limit is reached;
the tag will not end up being added to `unclosed_tags` because the queue
will never be flushed. So, now, if the `unclosed_tags` stack is empty,
`close_tag()` does nothing.
This change fixes a panic in the `limit_0` unit test.
@camelidcamelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 26, 2021
@camelid

camelid commented Aug 26, 2021

Copy link
Copy Markdown
MemberAuthor

I'll push up a fix for the test failures tomorrow.

@camelid
camelidforce-pushed the refactor-markdown-length-limit branch from 84fd81b to 4478eccCompareAugust 26, 2021 16:18
@camelidcamelid removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 26, 2021
@camelid

camelid commented Aug 28, 2021

Copy link
Copy Markdown
MemberAuthor

Ok, CI passed, this should be ready for a new review!

@GuillaumeGomez

GuillaumeGomez commented Aug 29, 2021

Copy link
Copy Markdown
Member

Thanks!

@bors: r+

@bors

bors commented Aug 29, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 4478ecc has been approved by GuillaumeGomez

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2021
@GuillaumeGomezGuillaumeGomez mentioned this pull request Aug 29, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 29, 2021
@bors
Auto merge ofrust-lang#88461- GuillaumeGomez:rollup-khgu0eb, r=Guil…
daa4dc9
…laumeGomez
Rollup of 13 pull requests
Successful merges:
- rust-lang#80543 (Notify when an `I-prioritize` issue is closed or reopened)
- rust-lang#83251 (Suggestion for call on immutable binding of mutable type)
- rust-lang#85534 (add rustc-demangle assertion on mangled symbol)
- rust-lang#88173 (Refactor Markdown length-limited summary implementation)
- rust-lang#88349 (Add const and static TAIT tests)
- rust-lang#88357 (add unsized coercion test)
- rust-lang#88381 (Handle stack_t.ss_sp type change for DragonFlyBSD)
- rust-lang#88387 (Remove vestigial rustfix tests.)
- rust-lang#88396 (Bump vulnerable crates)
- rust-lang#88407 (Fix formatting in release notes from 52a9883)
- rust-lang#88411 (Remove `Session.if_let_suggestions`)
- rust-lang#88417 (RELEASES.md: fix broken link)
- rust-lang#88419 (Fix code blocks color in Ayu theme)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit aa61575 into rust-lang:masterAug 29, 2021
@rustbotrustbot added this to the 1.56.0 milestone Aug 29, 2021
@camelid
camelid deleted the refactor-markdown-length-limit branch August 29, 2021 18:06
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GuillaumeGomezGuillaumeGomezGuillaumeGomez approved these changes
+1 more reviewer
@jyn514jyn514jyn514 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@GuillaumeGomezGuillaumeGomez

Labels

C-cleanupCategory: PRs that clean code up or issues documenting cleanup.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.56.0

Development

Successfully merging this pull request may close these issues.

7 participants

@camelid@rust-log-analyzer@GuillaumeGomez@bors@jyn514@rust-highfive@rustbot