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

fix #4938: i18n doesn't handle node.title correctly #4939

Merged
merged 3 commits into from
May 11, 2018
Merged

Conversation

shimizukawa
Copy link
Member

Feature or Bugfix

Purpose

  1. make gettext generates msgid for title of topic directive and contents directive
  2. make html with i18n catalog apply translated message even if msgid starts with 1.

Detail

see #4938

@codecov
Copy link

codecov bot commented May 6, 2018

Codecov Report

Merging #4939 into 1.7 will increase coverage by 0.03%.
The diff coverage is 95.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##              1.7    #4939      +/-   ##
==========================================
+ Coverage   81.96%   81.99%   +0.03%     
==========================================
  Files         282      287       +5     
  Lines       37594    37960     +366     
  Branches     5829     5893      +64     
==========================================
+ Hits        30813    31125     +312     
- Misses       5477     5523      +46     
- Partials     1304     1312       +8
Impacted Files Coverage Δ
sphinx/util/nodes.py 90.86% <100%> (+0.08%) ⬆️
tests/test_intl.py 98.76% <100%> (+0.09%) ⬆️
sphinx/transforms/i18n.py 91.09% <66.66%> (+0.14%) ⬆️
sphinx/search/__init__.py 95.11% <0%> (ø)
sphinx/__init__.py 60% <0%> (ø)
sphinx/quickstart.py 0% <0%> (ø)
sphinx/errors.py 68.42% <0%> (ø)
sphinx/apidoc.py 0% <0%> (ø)
sphinx/builders/html.py 82.66% <0%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb8c659...9677ddc. Read the comment docs.

@shimizukawa shimizukawa changed the base branch from master to 1.7 May 6, 2018 09:15
@shimizukawa shimizukawa self-assigned this May 6, 2018
@shimizukawa shimizukawa added this to the 1.7.5 milestone May 6, 2018
# see: http://docutils.sourceforge.net/docs/ref/doctree.html#structural-subelements
if isinstance(node, nodes.title):
# This generates: <section ...><title>msgstr</title></section>
msgstr = msgstr + '\n' + '-' * len(msgstr) * 2
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this code is good. I should refactor this sphinx.transform.i18n module later.

if isinstance(node, nodes.title) and node.source is None:
# Uncomment these lines after merging into master(1.8)
# logger.debug('[i18n] PATCH: %r to have source: %s',
# get_full_module_name(node), repr_domxml(node))
Copy link
Member Author

Choose a reason for hiding this comment

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

this comment lines will uncomment after merging master

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM with nits

@@ -132,6 +137,11 @@ def is_translatable(node):
META_TYPE_NODES = (
addnodes.meta,
)
ALLOWED_TRANSLATED_NODES = (
Copy link
Member

Choose a reason for hiding this comment

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

What is difference from translatable nodes?

On i18n transform, it is described as following:

# ignore block markups
if not isinstance(patch, ALLOWED_TRANSLATED_NODES):
    continue  # skip

Are these same meaning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not same.
When msgstr in po files contain block markup, if not isinstance(patch, ALLOWED_TRANSLATED_NODES): ignore such translations.

Copy link
Member

Choose a reason for hiding this comment

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

I feel it is confusable. Could you rename this please to more appropriate name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about defining it by naming rather than listing node classes in the code of i18n.py. So, I named ALLOWED_TRANSLATED_NODES to them, but this was not a property of being given a single name.
The nodes.title that added this time is a hacky code to handle in pair with the processing in i18n.py.
I'll revert this part and add a comment for each node classes.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@shimizukawa
Copy link
Member Author

I'll merge this after test passing

@shimizukawa shimizukawa merged commit 79650f5 into 1.7 May 11, 2018
@shimizukawa shimizukawa deleted the fix4938 branch May 11, 2018 11:51
shimizukawa added a commit that referenced this pull request May 11, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants