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 parsing of options with enabled option_emphasise_placeholders #10565

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Jun 17, 2022

If the option is enabled, the current code wrongly uses

if not args:
  continue

which skips adjustement of signode and firstname
at the very of the loop.

Fixes: #10366.

@AA-Turner
Copy link
Member

Can you add a test to ensure this behaviour doesn't regress?

A

@marxin marxin force-pushed the fix-parsing-with-option_emphasise_placeholders branch 3 times, most recently from 234b559 to 248ae64 Compare June 17, 2022 19:40
@marxin
Copy link
Contributor Author

marxin commented Jun 17, 2022

Can you add a test to ensure this behaviour doesn't regress?

Sure, I've just done that.

@tk0miya tk0miya added this to the 5.1.0 milestone Jun 19, 2022
@tk0miya
Copy link
Member

tk0miya commented Jun 19, 2022

It seems conflicted. Could you check it please?

If the option is enabled, the current code wrongly uses

if not args:
  continue

which skips adjustment of signode and firstname
at the very of the loop.

Fixes: sphinx-doc#10366.
@marxin marxin force-pushed the fix-parsing-with-option_emphasise_placeholders branch from 248ae64 to 664d9df Compare June 19, 2022 16:19
@marxin
Copy link
Contributor Author

marxin commented Jun 19, 2022

It seems conflicted. Could you check it please?

Sure, the conflicts are resolved now.

@AA-Turner AA-Turner merged commit ea46202 into sphinx-doc:5.x Jun 19, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2022
@marxin marxin deleted the fix-parsing-with-option_emphasise_placeholders branch September 9, 2022 10:29
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

3 participants