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

Integrate to the bootstrap process of rust-lang/rust #193

Merged
merged 1 commit into from
May 21, 2024

Conversation

dalance
Copy link
Contributor

@dalance dalance commented May 14, 2024

This PR changes the followings for rust-lang/rust#124731:

  • Export Gettext preprocessor
  • Fix "hidden lifetime parameters in types are deprecated" error
  • Downgrade the minimum required version of regex to 1.9.4

Closes #191

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2024

Codecov Report

Attention: Patch coverage is 6.45161% with 58 lines in your changes are missing coverage. Please review.

Project coverage is 89.74%. Comparing base (2b14491) to head (b44611b).

Files Patch % Lines
i18n-helpers/src/preprocessors/gettext.rs 0.00% 50 Missing ⚠️
i18n-helpers/src/bin/mdbook-gettext.rs 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
- Coverage   90.27%   89.74%   -0.54%     
==========================================
  Files          12       13       +1     
  Lines        3034     3052      +18     
  Branches     3034     3052      +18     
==========================================
  Hits         2739     2739              
- Misses        207      225      +18     
  Partials       88       88              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -19,7 +19,7 @@ mdbook.workspace = true
polib.workspace = true
pulldown-cmark = { version = "0.9.2", default-features = false }
pulldown-cmark-to-cmark = "11.2.0"
regex = "1.10.4"
regex = "1.9"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might get a PR from the dependabot, but we can then tell it to ignore the 1.10 releases.

Comment on lines 1 to 2
pub mod gettext;
pub use gettext::Gettext;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a fan of giving multiple public names for the same thing — could you instead make the module private?

Suggested change
pub mod gettext;
pub use gettext::Gettext;
mod gettext;
pub use gettext::Gettext;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed it.

Comment on lines +59 to +60
let gettext = Gettext;
if gettext.supports_renderer(renderer) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this work as well:

Suggested change
let gettext = Gettext;
if gettext.supports_renderer(renderer) {
if Gettext::supports_renderer(renderer) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't change the definition of supports_renderer and run because they are defined as methods of Preprocessor trait at https://docs.rs/mdbook/latest/mdbook/preprocess/trait.Preprocessor.html.

I can define another static method like impl_supports_renderer, and change it to Gettext::impl_supports_renderer,
but it may be redundant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see now! The syntax I used above would only work if the supports_renderer method had no &self argument.

Then let's merge this PR as it is — and then later change the code here to create a single Gettext value and pass this around. That would be a little more natural, I think (thought it would just be a small refactor).

Comment on lines +47 to +48
let gettext = Gettext;
let book = gettext.run(&ctx, book)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like above, I think we don't need the variable:

Suggested change
let gettext = Gettext;
let book = gettext.run(&ctx, book)?;
let book = Gettext::run(&ctx, book)?;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @dalance, can you try out this change and see if it compiles? If so, please add it to the PR. Thanks!

@mgeisler
Copy link
Collaborator

Hi @dalance, thanks a lot for the PR! I would like to merge it after the small conflicts are resolved.

@dalance
Copy link
Contributor Author

dalance commented May 16, 2024

Thank you for your review. I resolved conflict and fixed the module visibility.

@mgeisler mgeisler merged commit 5b855b0 into google:main May 21, 2024
7 checks passed
@mgeisler
Copy link
Collaborator

We should make a release with this change — @kdarkhan, are there any other changes or fixes which you would like us to wait for? I think we could get #195 merged and then do the release.

@kdarkhan
Copy link
Collaborator

We should make a release with this change — @kdarkhan, are there any other changes or fixes which you would like us to wait for? I think we could get #195 merged and then do the release.

Nothing from my side. I am ok with the release with 195 merged.

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.

Integrate to the bootstrap process of rust-lang/rust
4 participants