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

Jump to bottom

Allow emphasising placeholders in option directives #10366

Merged
merged 3 commits into from Jun 16, 2022
Merged

Allow emphasising placeholders in option directives #10366

merged 3 commits into from Jun 16, 2022

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Apr 19, 2022

Support parsing of "variable part" for option directives similarly
to samp roles.

Fixes: #9965.

This is a follow-up of #10001 which targeted 4.x branch. I included @jakobandersen which were suggested in:
https://github.com/jakobandersen/sphinx/tree/marxin-option_parse_variable_part.

Apart from that, I have to handle empty args in handle_signature function.

@jakobandersen
Copy link
Contributor

Looks good to me.

Apart from that, I have to handle empty args in handle_signature function.

Not exactly sure what you mean here. Do you have an example?

@jakobandersen jakobandersen added this to the 5.0.0 milestone Apr 23, 2022
@jakobandersen jakobandersen added type:enhancement enhance or introduce a new feature domains:std labels Apr 23, 2022
AA-Turner
AA-Turner reviewed Apr 23, 2022
View reviewed changes
sphinx/domains/std.py Outdated Show resolved Hide resolved
@marxin marxin force-pushed the option_parse_variable_part-v2 branch from a60357e to 552aeca Compare April 25, 2022 13:29
@marxin
Copy link
Contributor Author

marxin commented Apr 25, 2022

Looks good to me.

Thanks. Can you please mark it with a Github Approval?

Apart from that, I have to handle empty args in handle_signature function.

Not exactly sure what you mean here. Do you have an example?

Sure, happens for situation like: .. option:: -Wno-shift-overflow, -Wshift-overflow=n, -Wshift-overflow. Which is parsed in the following way:

optname: "-Wno-shift-overflow", args: ""
optname: "-Wshift-overflow", args: "=n"
optname: "-Wshift-overflow", args: ""

AA-Turner
AA-Turner reviewed Apr 25, 2022
View reviewed changes
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Some comments on the documentation.

An aside, perhaps the option could be named option_emphasise_placeholders or similar? parse_detailed doesn't tell me much about what is going on, and it isn't just the parsing that changes with this option, but the output too.

A

doc/usage/restructuredtext/domains.rst Outdated Show resolved Hide resolved
doc/usage/configuration.rst Outdated Show resolved Hide resolved
@marxin
Copy link
Contributor Author

marxin commented Apr 26, 2022

An aside, perhaps the option could be named option_emphasise_placeholders or similar? parse_detailed doesn't tell me much about what is going on, and it isn't just the parsing that changes with this option, but the output too.

Thanks, I welcome the suggested changes, what about @jakobandersen?

@marxin marxin force-pushed the option_parse_variable_part-v2 branch 2 times, most recently from 214665d to 57e3fab Compare May 9, 2022 08:18
@marxin
Copy link
Contributor Author

marxin commented May 9, 2022

An aside, perhaps the option could be named option_emphasise_placeholders or similar? parse_detailed doesn't tell me much about what is going on, and it isn't just the parsing that changes with this option, but the output too.

Well, I can imagine it can support more syntax for the future, so please let's use the more generic name option_emphasise_placeholders.

@marxin marxin force-pushed the option_parse_variable_part-v2 branch from 57e3fab to 28f97c2 Compare May 9, 2022 08:20
@marxin
Copy link
Contributor Author

marxin commented May 9, 2022

I would really like to see this in 5.x as I'm in the review process for almost 6 months.
Can you @AA-Turner and @jakobandersen make an approval of it? Thanks.

@AA-Turner
Copy link
Member

The implementation looks reasonable, although I still don't like 'detailed_parse', as users would likely want to configure any other 'detailed parsing' independently from this.

A

@marxin marxin force-pushed the option_parse_variable_part-v2 branch from 28f97c2 to 1b82384 Compare May 10, 2022 11:58
@marxin marxin changed the title Add option_detailed_parse config value. Add option_emphasise_placeholders config value. May 10, 2022
@marxin
Copy link
Contributor Author

marxin commented May 10, 2022

The implementation looks reasonable, although I still don't like 'detailed_parse', as users would likely want to configure any other 'detailed parsing' independently from this.

I don't have a problem with the option name suggestion, thus applied.

AA-Turner
AA-Turner approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Thanks @marxin!

A

@tk0miya tk0miya modified the milestones: 5.0.0, 5.x, 5.1.0 May 22, 2022
@marxin marxin force-pushed the option_parse_variable_part-v2 branch from 1b82384 to 786130c Compare May 25, 2022 07:29
@marxin @AA-Turner
Add option_emphasise_placeholders config value.
515c935 
Support parsing of "variable part" for option directives similarly
to samp roles.

Fixes: sphinx-doc#9965.

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Signed-off-by: Martin Liska <mliska@suse.cz>
@marxin marxin force-pushed the option_parse_variable_part-v2 branch from 786130c to 515c935 Compare June 9, 2022 20:45
@AA-Turner AA-Turner changed the title Add option_emphasise_placeholders config value. Add option_emphasise_placeholders config value Jun 16, 2022
@AA-Turner AA-Turner changed the title Add option_emphasise_placeholders config value Allow emphasising placeholders in option directives Jun 16, 2022
@AA-Turner AA-Turner merged commit f789148 into sphinx-doc:5.x Jun 16, 2022
@AA-Turner
Copy link
Member

Thanks for your perseverance @marxin, merged.

A

@marxin
Copy link
Contributor Author

marxin commented Jun 17, 2022

Thank you guys for merging this, really helps me.

@marxin marxin deleted the option_parse_variable_part-v2 branch June 17, 2022 07:25
marxin added a commit to marxin/sphinx that referenced this pull request Jun 17, 2022
@marxin
Fix parsion of options with enabled option_emphasise_placeholders
64775fe 
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: sphinx-doc#10366.
marxin added a commit to marxin/sphinx that referenced this pull request Jun 17, 2022
@marxin
Fix parsing of options with enabled option_emphasise_placeholders
8ed1955 
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 mentioned this pull request Jun 17, 2022
Merged
marxin added a commit to marxin/sphinx that referenced this pull request Jun 17, 2022
@marxin
Fix parsing of options with enabled option_emphasise_placeholders
bad6fb9 
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 added a commit to marxin/sphinx that referenced this pull request Jun 17, 2022
@marxin
Fix parsing of options with enabled option_emphasise_placeholders
234b559 
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 added a commit to marxin/sphinx that referenced this pull request Jun 17, 2022
@marxin
Fix parsing of options with enabled option_emphasise_placeholders
248ae64 
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 added a commit to marxin/sphinx that referenced this pull request Jun 19, 2022
@marxin
Fix parsing of options with enabled option_emphasise_placeholders
664d9df 
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.
jfbu added a commit that referenced this pull request Jun 26, 2022
@jfbu
Improve mark-up in docs (follow-up to PR #10366)
8ecbe8a
@CendioOssman CendioOssman mentioned this pull request Jul 1, 2022
Open
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AA-Turner AA-Turner AA-Turner approved these changes

Assignees
No one assigned
Labels
domains:std type:enhancement enhance or introduce a new feature
Projects
None yet
Milestone
5.1.0
Development

Successfully merging this pull request may close these issues.

Allow a variable part in ..option directives
4 participants
@marxin @jakobandersen @AA-Turner @tk0miya