-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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-110631: Fix wrong reST markup and list numbers. #110885
Open
ezio-melotti
wants to merge
2
commits into
python:main
Choose a base branch
from
ezio-melotti:fix-ind-rem
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+7
−7
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1107,7 +1107,7 @@ subject value: | |||||||||
If only keyword patterns are present, they are processed as follows, | ||||||||||
one by one: | ||||||||||
|
||||||||||
I. The keyword is looked up as an attribute on the subject. | ||||||||||
1. The keyword is looked up as an attribute on the subject. | ||||||||||
|
||||||||||
* If this raises an exception other than :exc:`AttributeError`, the | ||||||||||
exception bubbles up. | ||||||||||
|
@@ -1119,13 +1119,13 @@ subject value: | |||||||||
pattern fails; if this succeeds, the match proceeds to the next keyword. | ||||||||||
|
||||||||||
|
||||||||||
II. If all keyword patterns succeed, the class pattern succeeds. | ||||||||||
2. If all keyword patterns succeed, the class pattern succeeds. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
If any positional patterns are present, they are converted to keyword | ||||||||||
patterns using the :data:`~object.__match_args__` attribute on the class | ||||||||||
``name_or_attr`` before matching: | ||||||||||
|
||||||||||
I. The equivalent of ``getattr(cls, "__match_args__", ())`` is called. | ||||||||||
1. The equivalent of ``getattr(cls, "__match_args__", ())`` is called. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
* If this raises an exception, the exception bubbles up. | ||||||||||
|
||||||||||
|
@@ -1143,8 +1143,8 @@ subject value: | |||||||||
|
||||||||||
.. seealso:: :ref:`class-pattern-matching` | ||||||||||
|
||||||||||
II. Once all positional patterns have been converted to keyword patterns, | ||||||||||
the match proceeds as if there were only keyword patterns. | ||||||||||
2. Once all positional patterns have been converted to keyword patterns, | ||||||||||
the match proceeds as if there were only keyword patterns. | ||||||||||
Comment on lines
+1147
to
+1148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
For the following built-in types the handling of positional subpatterns is | ||||||||||
different: | ||||||||||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect the different numbering style (Roman numerals) was used here to differentiate this inner numbered list from the outer one.
Before
After
Can we keep it them as Roman numerals, but fix the formatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sphinx <7 doesn't seem to support them, so they are actually just regular paragraphs starting with
I. ...
. Letters likea. b. c. ...
are also not supported, so we only have numbers left and have to rely on the indentation to distinguish the levels.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
Well, we need to keep support for older Sphinx for the benefit of Linux distros (and are testing it on CI), but can we use Sphinx 7 for our actual main build and deploy?
The Roman numerals won't cause any errors for for Sphinx <7, and they'll be better rendered for for Sphinx 7.
Re: #99380 and cc @AA-Turner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should be able to fix the indentation/rendering of the nested lists while keeping the roman numerals, the only issue is that since they are rendered as
<p>
s rather than<li>
s, it's technically incorrect (at least on Sphinx <7). Not sure if that matters though (maybe for screen readers or similar cases?).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually if I fix it for <7, once we upgrade to 7 it will break again.
On <7 I could do:
which will be seen as a paragraph followed by a list, but on 7 it would have to be:
since it will be seen as a list item followed by a sublist that needs to be indented.
If I leave the indentation on <7, it will render an additional blockquote around the sublist, and it will cause
sphinx-lint
to complain, so switching to numbers might still be the best compromise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind too much if the display isn't perfect for <7, as long as it still builds and looks reasonable.
Then we can use 7 for our deploys, and sphinx-lint will be happy too.
Would that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sphinx-lint
will be sad on 7 too, since the checker currently ignores alphabetic lists (like Sphinx 6 does) and sees this as an incorrectly indented list under a paragraph, regardless of the Sphinx version used.I tried adding support for alphabetic lists, but it's a can of worms with many false positives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe we should ignore the Sphinx Lint warning here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked into this in a while, but if possible we should find a solution that is both rendered correctly and that is not reported by
sphinx-lint
as error.Fixing
sphinx-lint
is also an option if the error is reported mistakenly, but avoiding alphabetic lists and roman numerals might still be a simpler solution.