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

gh-97607: Fix content parsing in the impl-detail reST directive #97652

Merged
merged 3 commits into from
Oct 2, 2022

Conversation

CAM-Gerlach
Copy link
Member

@CAM-Gerlach CAM-Gerlach commented Sep 29, 2022

This fixes #97607 with the custom impl-detail reST directive in the docs, that results in the text being untranslated in translations (e.g. python/python-docs-zh-tw#187 ) when it isn't written as a separate content paragraph under the directive (as opposed to with no or one line break after the directive start ::, as opposed to two).

In particular, currently the directive treats text in the same paragraph as the directive start by capturing it in an optional argument and manually processing it, which results in missing metadata (e.g. raw source, file and line number) in the parsed doctree nodes (that in turn breaks the translation code that requires this metadata). This PR fixes the directive to use the same internal logic as the similar builtin docutils/Sphinx directives (e.g. the various admonition types), taking no arguments and just treating that text as directive content by default, regardless of the number of line breaks.

All three forms (0, 1, and 2 line breaks) are used for e.g. note:: in the os module doc source, and all three render correctly in the translated os module docs, and I also confirmed that the parsed doctree structure, content and attributes are now identical for all three to that previously generated for separate paragraphs, and the untranslated HTML is byte-identical both between source forms and before and after this change. In addition to fixing the issue, this also simplifies the directive code and logic.

Additionally, I also was able to eliminate the related unused branch from the directive code that handled the situation where the directive is used alone with no content at all. This isn't used anywhere in the docs and seems to lack a clear use case, and its use would be more likely to just an unintentional mistake (e.g. due to an indentation mismatch). The behavior is now likewise consistent with the aforementioned built-in directive, being flagged as an error at build time, and the code/logic is further simplified from two (originally three) code paths to one.

@StevenHsuYL , could you test to confirm that this PR fixes the issue you experienced in python/python-docs-zh-tw#187 and reported in #97607 ? Thanks!

Fixes #97607
Closes #97635

This does not change the (untranslated) output,
but ensures that the doctree node metadata is correct.
which fixes pythongh-97607 with the text not being translated.
It also simplifies the code and logic
and makes it consistant with the docutils built-in directives.
This is not used anywhere in the docs and lacks a clear use case,
and is more likely a mistake which is now flagged at build time.
This simplifies the logic from two code paths to one,
and makes the behavior consistant with similar built-in directives
(e.g. the various admonition types).
@CAM-Gerlach CAM-Gerlach added docs Documentation in the Doc dir skip news needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Sep 29, 2022
@CAM-Gerlach
Copy link
Member Author

FYI, @pablogsal in case this isn't backported to the release branch by default as a docs change, this will need to be to ensure translations for these blocks work, given the reported bug.

Doc/tools/extensions/pyspecific.py Outdated Show resolved Hide resolved
@AA-Turner
Copy link
Member

I agree zero-content impl-detail doesn't make sense.

A

Copy link
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

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

@StevenHsuYL , could you test to confirm that this PR fixes the issue you experienced in python/python-docs-zh-tw#187 and reported in #97607 ? Thanks!

I tested this and can confirm that after updating Doc/library/queue.rst(including the change suggested by @AA-Turner), the translation of the implementation detail is included correctly:
image

I also tried rebuilding the .po for Doc/library/readline.rst and it added the implementation note that was previously missing:

Expand diff
$ git diff library/readline.po
diff --git a/library/readline.po b/library/readline.po
index 6781a16..cfd0e1c 100644
--- a/library/readline.po
+++ b/library/readline.po
@@ -7,7 +7,7 @@ msgid ""
 msgstr ""
 "Project-Id-Version: Python 3.10\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2021-10-26 16:47+0000\n"
+"POT-Creation-Date: 2022-09-30 04:06+0200\n"
 "PO-Revision-Date: 2018-05-23 16:09+0000\n"
 "Last-Translator: Adrian Liaw <[email protected]>\n"
 "Language-Team: Chinese - TAIWAN (https://github.com/python/python-docs-zh-"
@@ -205,6 +205,12 @@ msgid ""
 "when true, enables auto history, and that when false, disables auto history."
 msgstr ""
 
+#: ../../library/readline.rst:188
+msgid ""
+"Auto history is enabled by default, and changes to this do not persist "
+"across multiple sessions."
+msgstr ""
+
 #: ../../library/readline.rst:193
 msgid "Startup hooks"
 msgstr ""

@CAM-Gerlach
Copy link
Member Author

Warning, treated as error:
Unknown platform(s) or syntax 'systems with POSIX threads' in '.. availability:: Windows, systems with POSIX threads.', see > /home/hchsu/github/cpython/Doc/tools/extensions/pyspecific.py:Availability.known_platforms for a set known platforms.

That's an error from the availability custom directive, unrelated to the impl-detail custom directive modified here. Specifically, you're seeing that because that directive was changed in recent versions to be more strict with the input it accepts (only names of known platforms), and evidently the corresponding docs changes weren't backported. If you're going to apply them manually, just copy the ImplementationDetail class, which is the only one that was actually changed here (or merge this branch with 3.10 with Git).

@StevenHsuYL
Copy link
Contributor

Thanks for the reply.
I applied the change in this PR to my 3.10 branch, which works well for translation page generation.
The described issue has disappeared.

Thanks a lot!

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Sep 30, 2022

Great, thanks! I've also tested again with the suggested changes locally to confirm.

@ezio-melotti ezio-melotti merged commit e8165d4 into python:main Oct 2, 2022
@miss-islington
Copy link
Contributor

Thanks @CAM-Gerlach for the PR, and @ezio-melotti for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-97723 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 2, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 2, 2022
…pythonGH-97652)

* Don't parse content as arg in the impl-detail directive

This does not change the (untranslated) output,
but ensures that the doctree node metadata is correct.
which fixes pythongh-97607 with the text not being translated.
It also simplifies the code and logic
and makes it consistant with the docutils built-in directives.

* Remove unused branch from impl-detail directive handling no-content case

This is not used anywhere in the docs and lacks a clear use case,
and is more likely a mistake which is now flagged at build time.
This simplifies the logic from two code paths to one,
and makes the behavior consistant with similar built-in directives
(e.g. the various admonition types).

* Further simplify impl-detail reST directive code
(cherry picked from commit e8165d4)

Co-authored-by: C.A.M. Gerlach <[email protected]>
@bedevere-bot
Copy link

GH-97724 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 2, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 2, 2022
…pythonGH-97652)

* Don't parse content as arg in the impl-detail directive

This does not change the (untranslated) output,
but ensures that the doctree node metadata is correct.
which fixes pythongh-97607 with the text not being translated.
It also simplifies the code and logic
and makes it consistant with the docutils built-in directives.

* Remove unused branch from impl-detail directive handling no-content case

This is not used anywhere in the docs and lacks a clear use case,
and is more likely a mistake which is now flagged at build time.
This simplifies the logic from two code paths to one,
and makes the behavior consistant with similar built-in directives
(e.g. the various admonition types).

* Further simplify impl-detail reST directive code
(cherry picked from commit e8165d4)

Co-authored-by: C.A.M. Gerlach <[email protected]>
miss-islington added a commit that referenced this pull request Oct 2, 2022
…7652)

* Don't parse content as arg in the impl-detail directive

This does not change the (untranslated) output,
but ensures that the doctree node metadata is correct.
which fixes gh-97607 with the text not being translated.
It also simplifies the code and logic
and makes it consistant with the docutils built-in directives.

* Remove unused branch from impl-detail directive handling no-content case

This is not used anywhere in the docs and lacks a clear use case,
and is more likely a mistake which is now flagged at build time.
This simplifies the logic from two code paths to one,
and makes the behavior consistant with similar built-in directives
(e.g. the various admonition types).

* Further simplify impl-detail reST directive code
(cherry picked from commit e8165d4)

Co-authored-by: C.A.M. Gerlach <[email protected]>
miss-islington added a commit that referenced this pull request Oct 2, 2022
…7652)

* Don't parse content as arg in the impl-detail directive

This does not change the (untranslated) output,
but ensures that the doctree node metadata is correct.
which fixes gh-97607 with the text not being translated.
It also simplifies the code and logic
and makes it consistant with the docutils built-in directives.

* Remove unused branch from impl-detail directive handling no-content case

This is not used anywhere in the docs and lacks a clear use case,
and is more likely a mistake which is now flagged at build time.
This simplifies the logic from two code paths to one,
and makes the behavior consistant with similar built-in directives
(e.g. the various admonition types).

* Further simplify impl-detail reST directive code
(cherry picked from commit e8165d4)

Co-authored-by: C.A.M. Gerlach <[email protected]>
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Oct 2, 2022
…python#97652)

* Don't parse content as arg in the impl-detail directive

This does not change the (untranslated) output,
but ensures that the doctree node metadata is correct.
which fixes pythongh-97607 with the text not being translated.
It also simplifies the code and logic
and makes it consistant with the docutils built-in directives.

* Remove unused branch from impl-detail directive handling no-content case

This is not used anywhere in the docs and lacks a clear use case,
and is more likely a mistake which is now flagged at build time.
This simplifies the logic from two code paths to one,
and makes the behavior consistant with similar built-in directives
(e.g. the various admonition types).

* Further simplify impl-detail reST directive code
carljm added a commit to carljm/cpython that referenced this pull request Oct 3, 2022
* main: (2069 commits)
  pythongh-96512: Move int_max_str_digits setting to PyConfig (python#96944)
  pythongh-94808: Coverage: Check picklablability of calliter (python#95923)
  pythongh-94808: Add test coverage for PyObject_HasAttrString (python#96627)
  pythongh-94732: Fix KeyboardInterrupt race in asyncio run_forever() (python#97765)
  Fix typos in `bltinmodule.c`. (pythonGH-97766)
  pythongh-94808: `_PyLineTable_StartsLine` was not used (pythonGH-96609)
  pythongh-97681: Remove Tools/demo/ directory (python#97682)
  Fix typo in unittest docs (python#97742)
  pythongh-97728: Argument Clinic: Fix uninitialized variable in the Py_UNICODE converter (pythonGH-97729)
  pythongh-95913: Fix PEP number in PEP 678 What's New ref label (python#97739)
  pythongh-95913: Copyedit/improve New Modules What's New section (python#97721)
  pythongh-97740: Fix bang in Sphinx C domain ref target syntax (python#97741)
  pythongh-96819: multiprocessing.resource_tracker: check if length of pipe write <= 512 (python#96890)
  pythongh-97706: multiprocessing tests: Delete unused variable `rand` (python#97707)
  pythonGH-85447: Clarify docs about awaiting future multiple times (python#97738)
  [docs] Update logging cookbook with recipe for using a logger like an output… (pythonGH-97730)
  pythongh-97607: Fix content parsing in the impl-detail reST directive (python#97652)
  pythongh-95975: Move except/*/finally ref labels to more precise locations (python#95976)
  pythongh-97591: In `Exception.__setstate__()` acquire strong references before calling `tp_hash` slot (python#97700)
  pythongh-95588: Drop the safety claim from `ast.literal_eval` docs. (python#95919)
  ...
pablogsal pushed a commit that referenced this pull request Oct 22, 2022
…7652)

* Don't parse content as arg in the impl-detail directive

This does not change the (untranslated) output,
but ensures that the doctree node metadata is correct.
which fixes gh-97607 with the text not being translated.
It also simplifies the code and logic
and makes it consistant with the docutils built-in directives.

* Remove unused branch from impl-detail directive handling no-content case

This is not used anywhere in the docs and lacks a clear use case,
and is more likely a mistake which is now flagged at build time.
This simplifies the logic from two code paths to one,
and makes the behavior consistant with similar built-in directives
(e.g. the various admonition types).

* Further simplify impl-detail reST directive code
(cherry picked from commit e8165d4)

Co-authored-by: C.A.M. Gerlach <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lack of a blank line after .. impl-detail::
7 participants