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

extend option directive syntax #10840

Merged
merged 1 commit into from Sep 27, 2022

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Sep 20, 2022

The motivation of pull request is a documentation syntax we use in GCC docs (working on the porting):

.. option:: -Wimplicit-fallthrough

  :option:`-Wimplicit-fallthrough` is the same as :option:`-Wimplicit-fallthrough=3`
  and :option:`-Wno-implicit-fallthrough` is the same as
  :option:`-Wimplicit-fallthrough=0`.

.. option:: -Wno-implicit-fallthrough

  Default setting; overrides :option:`-Wimplicit-fallthrough`.

.. option:: -Wimplicit-fallthrough#{n}

  Warn when a switch case falls through.  For example:
  ...

.. option:: -Wimplicit-fallthrough#0

   Disable it altogether.

.. option:: -Wimplicit-fallthrough#1

   Level 1

.. option:: -Wimplicit-fallthrough#2

   Level 2

.. option:: -Wimplicit-fallthrough#3

   Level 3

  ...

Try :option:`-Wimplicit-fallthrough=` or :option:`-Wimplicit-fallthrough` or :option:`-Wimplicit-fallthrough=n`

As seen, we basically need to distinguish:

  1. -Woption and -Woption= directives
  2. -Woption=value1, -Woption=value2, ...

Where it would be handy to reference to any of the aforementioned versions. I came up with a new syntax # that is a non-breaking version of =. Is the suggestion something the community would accept?

Example:
https://splichal.eu/tmp/demo2/html/

@marxin marxin marked this pull request as draft September 20, 2022 10:25
@marxin
Copy link
Contributor Author

marxin commented Sep 26, 2022

A bit of context: as you likely know I'm working on transition of th GCC compiler documentation from Texinfo to Sphinx and this feature would help you reference option much better. As seen, we commonly document option values.

Is the Sphinx community interested in the feature? I can add more tests and document it.
What do you think @AA-Turner, @tk0miya ?

@AA-Turner
Copy link
Member

My concern is about making the option directive too complicated to maintain for quite a niche usecase.

Is it possible in your conversion work to implement a custom option directive that works for GCC, or a flag to the standard option directive that hooks into your extra processing? If there are constraints that GCC must use only vanilla Sphinx then those routes aren't useable, clearly.

Is the following style usable?

.. option:: -Wimplicit-fallthrough

   Warn when a switch case falls through. Legal values are:
  
   - 0: Disable it altogether.
   - 1: Level 1
   - 2: Level 2
   - 3: Level 3

   :option:`-Wimplicit-fallthrough` is the same as :option:`-Wimplicit-fallthrough=3`
   and :option:`-Wno-implicit-fallthrough` is the same as
   :option:`-Wimplicit-fallthrough=0`.

.. option:: -Wno-implicit-fallthrough

  Default setting; overrides :option:`-Wimplicit-fallthrough`.

Try :option:`-Wimplicit-fallthrough=` or :option:`-Wimplicit-fallthrough` or :option:`-Wimplicit-fallthrough=n`

This would only require changing the option role to ignore = and anything after =.

A

@marxin
Copy link
Contributor Author

marxin commented Sep 26, 2022

My concern is about making the option directive too complicated to maintain for quite a niche usecase.

I do share the concert about the maintainability of the functionality.

Is it possible in your conversion work to implement a custom option directive that works for GCC, or a flag to the standard option directive that hooks into your extra processing? If there are constraints that GCC must use only vanilla Sphinx then those routes aren't useable, clearly.

We would like to support only vanilla Sphinx.

Is the following style usable?

.. option:: -Wimplicit-fallthrough

   Warn when a switch case falls through. Legal values are:
  
   - 0: Disable it altogether.
   - 1: Level 1
   - 2: Level 2
   - 3: Level 3

   :option:`-Wimplicit-fallthrough` is the same as :option:`-Wimplicit-fallthrough=3`
   and :option:`-Wno-implicit-fallthrough` is the same as
   :option:`-Wimplicit-fallthrough=0`.

.. option:: -Wno-implicit-fallthrough

  Default setting; overrides :option:`-Wimplicit-fallthrough`.

Try :option:`-Wimplicit-fallthrough=` or :option:`-Wimplicit-fallthrough` or :option:`-Wimplicit-fallthrough=n`

Yes, that's a feasible solution for us, I think.

This would only require changing the option role to ignore = and anything after =.

Yep. That would be a pretty small change, let me push it.

@marxin marxin force-pushed the extend-option-directive-syntax branch from 9f0d216 to 38eae9f Compare September 26, 2022 18:43
@marxin marxin marked this pull request as ready for review September 26, 2022 18:43
@marxin marxin force-pushed the extend-option-directive-syntax branch 4 times, most recently from 2592cfd to ac32bbe Compare September 27, 2022 10:37
@AA-Turner
Copy link
Member

Please merge 5.x to fix mypy

@AA-Turner AA-Turner added this to the 5.3.0 milestone Sep 27, 2022
@AA-Turner
Copy link
Member

Please also add a CHANGES entry

@marxin
Copy link
Contributor Author

marxin commented Sep 27, 2022

Both done now.

One can cross-reference an option value: :option:`--module=foobar`.
@marxin marxin force-pushed the extend-option-directive-syntax branch from d631093 to 9dee9f7 Compare September 27, 2022 17:45
@AA-Turner AA-Turner merged commit 29e6ada into sphinx-doc:5.x Sep 27, 2022
@AA-Turner
Copy link
Member

I don't like the GitHub UI where you're not prompted to review the commit message if your last merge was a "rebase and merge"... for posterity I was going to use:

Allow cross referencing options with values (#10840)

This change means that text following `=` is ignored when searching for a
corresponding option directive to an option cross reference role.

A

@marxin marxin deleted the extend-option-directive-syntax branch September 27, 2022 19:25
@marxin
Copy link
Contributor Author

marxin commented Sep 27, 2022

I like the wording improvement, please commit it.

marxin added a commit to marxin/sphinx that referenced this pull request Sep 29, 2022
This supports a commonly used format when it comes to optional argument:
-fauto-profile[=path], where `path` is optional part.

Extends: sphinx-doc#10840
marxin added a commit to marxin/sphinx that referenced this pull request Sep 29, 2022
This supports a commonly used format when it comes to optional argument:
-fauto-profile[=path], where `path` is optional part. Plus it support
a format when space is used: -fauto-profile path.

Extends: sphinx-doc#10840
marxin added a commit to marxin/sphinx that referenced this pull request Sep 29, 2022
This supports a commonly used format when it comes to optional argument:
-fauto-profile[=path], where `path` is optional part. Plus it supports
a format when space is used: -fauto-profile path.

Extends: sphinx-doc#10840
marxin added a commit to marxin/sphinx that referenced this pull request Oct 1, 2022
This supports a commonly used format when it comes to optional argument:
-fauto-profile[=path], where `path` is optional part. Plus it supports
a format when space is used: -fauto-profile path.

Extends: sphinx-doc#10840
marxin added a commit to marxin/sphinx that referenced this pull request Oct 2, 2022
This supports a commonly used format when it comes to optional argument:
-fauto-profile[=path], where `path` is optional part. Plus it supports
a format when space is used: -fauto-profile path.

Extends: sphinx-doc#10840
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
marxin added a commit to marxin/sphinx that referenced this pull request Oct 2, 2022
This supports a commonly used format when it comes to optional argument:
-fauto-profile[=path], where `path` is optional part. Plus it supports
a format when space is used: -fauto-profile path.

Extends: sphinx-doc#10840
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
marxin added a commit to marxin/sphinx that referenced this pull request Oct 2, 2022
This supports a commonly used format when it comes to optional argument:
-fauto-profile[=path], where `path` is optional part. Plus it supports
a format when space is used: -fauto-profile path.

Extends: sphinx-doc#10840
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
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