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

Make xgettext respect granularity for sub-chapter messages #215

Merged
merged 3 commits into from
Jul 23, 2024

Conversation

carreter
Copy link
Contributor

Related to #171.

While trying to refresh the Spanish translation of Comprehensive Rust, @henrif75 noticed that the messages.pot file I generated had line numbers that shouldn't be there (see google/comprehensive-rust#2120).

Digging around in the code, I noticed that the xgettext code was ignoring the granularity for sub-chapters. This PR fixes that!

@carreter carreter changed the title Make xgettext espect granularity for sub-chapter messages Make xgettext respect granularity for sub-chapter messages Jul 19, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2024

Codecov Report

Attention: Patch coverage is 96.55172% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.02%. Comparing base (68d2e29) to head (5cddc82).
Report is 6 commits behind head on main.

Files Patch % Lines
i18n-helpers/src/xgettext.rs 96.55% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #215      +/-   ##
==========================================
- Coverage   86.99%   85.02%   -1.98%     
==========================================
  Files          15       15              
  Lines        3229     3398     +169     
  Branches     3229     3398     +169     
==========================================
+ Hits         2809     2889      +80     
- Misses        327      413      +86     
- Partials       93       96       +3     

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

@kdarkhan
Copy link
Collaborator

Thanks for the fix, @carreter! Can you add or update one of the existing test cases which will catch this bug please?

I think it will be good to have this tested to ensure we don't break it in the future.

@carreter
Copy link
Contributor Author

Added the test as requested @kdarkhan !

@kdarkhan
Copy link
Collaborator

Please fix formatting. LGTM otherwise.

@kdarkhan kdarkhan merged commit 1da250d into google:main Jul 23, 2024
7 checks passed
@kdarkhan
Copy link
Collaborator

I added the fix on top of your PR, thanks for the contribution.

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.

3 participants