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

Add content_offset parameter to nested_parse_with_titles #11147

Merged

Conversation

jbms
Copy link
Contributor

@jbms jbms commented Jan 22, 2023

Feature or Bugfix

  • Bugfix

Purpose

Previously, nested_parse_with_titles always passed 0 as the input offset when invoking nested_parse. When parsing the content of a directive, as is a common use case for nested_parse_with_titles, this leads to incorrect source file/line number information, as it does not take into account the directive's content_offset, which is always non-zero.

This issue affects all object descriptions due to #10887. It also affects the ifconfig extension.

The py:module and js:module directives employed a workaround for this issue, by wrapping the calls to nested_parse_with_title with switch_source_input. That worked, but was more complicated (and likely less efficient) than necessary.

This commit adds an optional content_offset parameter to nested_parse_with_titles, and fixes callers to pass the appropriate content offset when needed.

This commit eliminates the now-unnecessary calls to switch_source_input and instead specifies the correct content_offset.

@jbms jbms force-pushed the fix-nested-parse-with-titles-source-info branch from f69851a to 3b30989 Compare January 22, 2023 05:38
Previously, `nested_parse_with_titles` always passed `0` as the input
offset when invoking `nested_parse`.  When parsing the content of a
directive, as is a common use case for `nested_parse_with_titles`,
this leads to incorrect source file/line number information, as it
does not take into account the directive's `content_offset`, which is
always non-zero.

This issue affects *all* object descriptions due to sphinx-doc#10887.  It also
affects the `ifconfig` extension.

The `py:module` and `js:module` directives employed a workaround for
this issue, by wrapping the calls to `nested_parse_with_title` with
`switch_source_input`.  That worked, but was more complicated (and
likely less efficient) than necessary.

This commit adds an optional `content_offset` parameter to
`nested_parse_with_titles`, and fixes callers to pass the appropriate
content offset when needed.

This commit eliminates the now-unnecessary calls to
`switch_source_input` and instead specifies the correct `content_offset`.
@jbms jbms force-pushed the fix-nested-parse-with-titles-source-info branch from 3b30989 to 7879ab1 Compare January 22, 2023 05:39
@jbms
Copy link
Contributor Author

jbms commented Jan 22, 2023

Note: An alternative fix would be to modify nested_parse_with_titles to always invoke switch_source_input. That would avoid the need for the content_offset parameter and any changes to callers of this function. But it would introduce at least some unnecessary added overhead from constructing a new StateMachine object.

jbms added a commit to jbms/sphinx-immaterial that referenced this pull request Jan 22, 2023
jbms added a commit to jbms/sphinx-immaterial that referenced this pull request Jan 22, 2023
@jfbu
Copy link
Contributor

jfbu commented Jan 22, 2023

merging latest master update into this, for CI checks

@jbms
Copy link
Contributor Author

jbms commented Jan 23, 2023

After the merge, CI seems to pass.

@jbms
Copy link
Contributor Author

jbms commented Feb 12, 2023

@jfbu It would be great if this fix can be merged soon. What do you think?

@jfbu
Copy link
Contributor

jfbu commented Feb 12, 2023

@jfbu It would be great if this fix can be merged soon. What do you think?

@jbms it looks good to me but I am the LaTeX contractor here so pinging @AA-Turner

@jbms
Copy link
Contributor Author

jbms commented Feb 12, 2023

@jfbu It would be great if this fix can be merged soon. What do you think?

@jbms it looks good to me but I am the LaTeX contractor here so pinging @AA-Turner

Oh, I see. Unfortunately he said he was out until Easter: #11116 (comment)

@AA-Turner AA-Turner changed the title Add content_offset parameter to nested_parse_with_titles Add content_offset parameter to nested_parse_with_titles Feb 15, 2023
@AA-Turner AA-Turner merged commit 8de6638 into sphinx-doc:master Feb 15, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2023
@AA-Turner AA-Turner added this to the 6.2.0 milestone Apr 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants