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

Update markdown-it version #8274

Merged
merged 2 commits into from
Jan 15, 2022
Merged

Update markdown-it version #8274

merged 2 commits into from
Jan 15, 2022

Conversation

thaliaarchi
Copy link
Contributor

@thaliaarchi thaliaarchi commented Jan 13, 2022

Fixes improperly-escaped pipes in Markdown tables for the documentation of bad_bit_mask and ineffective_bit_mask. A column pipe takes precedence over inline code markers, so some back ticks are displayed literally and the pipes need to be escaped. I found no other occurrences of the same problem when searching rust-clippy by \|.*`.*\|.

changelog: none

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @camsteffen (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 13, 2022
@camsteffen
Copy link
Contributor

Hmm the docs already look right to me at least on the website. Are you seeing it rendered incorrectly somewhere else?

@thaliaarchi
Copy link
Contributor Author

thaliaarchi commented Jan 14, 2022

The incorrectly-rendered tables show on my Firefox instance, but not any other browser. I did some digging and it turns out the culprit is my LocalCDN extension, which substitutes the CDNJS-hosted markdown-it (7.0.0) with a local version (12.3.0). If I disable LocalCDN for the Clippy docs, then it renders as expected.

This reveals the larger issue is that the markdown-it version needs updating. The version has not been updated since it was introduced.

For comparison, I have tested the tables in question in VS Code (which also uses markdown-it) and on GitHub and both render the same as the current markdown-it version. For consistency, I think we ought to update the version.

The bad_bit_mask table (without my escaping) rendered by GitHub:

Comparison Bit Op Example is always Formula
== or != & x & 2 == 3 false c & m != c
< or >= & x & 2 < 3 true m < c
> or <= & x & 1 > 1 false m <= c
== or != ` ` `x 1 == 0`
< or >= ` ` `x 1 < 1`
<= or > ` ` `x 1 > 0`

@thaliaarchi thaliaarchi changed the title Escape pipes in Markdown tables Update markdown-it version Jan 14, 2022
@thaliaarchi
Copy link
Contributor Author

The other dependencies could be updated as follows:

angular.js: 1.4.12 -> 1.8.2
highlight.js: 9.5.0 -> 11.4.0
markdown-it: 7.0.0 -> 12.3.2
twitter-bootstrap: 3.3.6 -> 4.6.1

I reviewed the changelogs for highlight.js and markdown-it and it seems that those could be simply updated without any other changes.

@camsteffen
Copy link
Contributor

In that case do you want to upgrade those two in this PR?

The other upgrades are probably worthwhile as well but those can be separate PRs and also should wait for #8070.

I also wonder if we should move to Angular (not AngularJS) or perhaps Vue but that's another discussion.

@thaliaarchi thaliaarchi changed the title Update markdown-it version Update markdown-it and highlight.js versions Jan 14, 2022
@thaliaarchi
Copy link
Contributor Author

I've upgraded markdown-it and highlight.js and I agree that AngularJS and Bootstrap are separate issues.

I have not tested it on my end as I am not sure of your pipeline for generating the GH Pages tree.

@camsteffen
Copy link
Contributor

Try cargo dev serve to test

@thaliaarchi
Copy link
Contributor Author

thaliaarchi commented Jan 14, 2022

The markdown-it fixes look to work properly,

Bumping the highlight.js version messes with the highlighting colors though. The page pulls in a custom highlight.css theme from rust-lang/mdBook, which overrides some of the styles from the github.min.css highlight.js theme. It looked fine before, but there must have been changes since then in highlight.js with the GitHub theme or class names. If either github.min.css or highlight.css are disabled, then it looks fine, though highlight.css is required for the color-theme-changing feature.

I've reverted the highlight.js version bump because I don't want to fix the bespoke themes in mdBook too. Interestingly, mdBook upgraded highlight.js to v11 last year, but reverted shortly thereafter.

@thaliaarchi thaliaarchi changed the title Update markdown-it and highlight.js versions Update markdown-it version Jan 14, 2022
@camsteffen
Copy link
Contributor

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 15, 2022

📌 Commit 65c072d has been approved by camsteffen

@bors
Copy link
Contributor

bors commented Jan 15, 2022

⌛ Testing commit 65c072d with merge 27845a9...

@bors
Copy link
Contributor

bors commented Jan 15, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 27845a9 to master...

@bors bors merged commit 27845a9 into rust-lang:master Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants