Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-110631: Fix wrong reST markup and list numbers.#110885

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
ezio-melotti wants to merge2 commits intopython:main
base:main
Choose a base branch
Loading
fromezio-melotti:fix-ind-rem

Conversation

ezio-melotti
Copy link
Member

@ezio-melottiezio-melotti commentedOct 15, 2023
edited by github-actionsbot
Loading

@@ -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.
Copy link
Member

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

image

After

image

Can we keep it them as Roman numerals, but fix the formatting?

Copy link
MemberAuthor

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 withI. .... 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.

Copy link
Member

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.

encukou reacted with thumbs up emoji
Copy link
MemberAuthor

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?).

Copy link
MemberAuthor

@ezio-melottiezio-melottiOct 16, 2023
edited
Loading

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:

I. The keyword is looked up as an attribute on the subject.* ...* ...

which will be seen as a paragraph followed by a list, but on 7 it would have to be:

I. The keyword is looked up as an attribute on the subject.   * ...   * ...

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 causesphinx-lint to complain, so switching to numbers might still be the best compromise.

Copy link
Member

@hugovkhugovkOct 17, 2023
edited
Loading

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?

encukou reacted with thumbs up emoji
Copy link
MemberAuthor

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.

Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. The keyword is looked up as an attribute on the subject.
I. The keyword is looked up as an attribute on the subject.

Copy link
MemberAuthor

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 bysphinx-lint as error.
Fixingsphinx-lint is also an option if the error is reported mistakenly, but avoiding alphabetic lists and roman numerals might still be a simpler solution.

@hugovkhugovk removed the needs backport to 3.11only security fixes labelApr 11, 2024
@serhiy-storchakaserhiy-storchaka added the needs backport to 3.13bugs and security fixes labelMay 9, 2024
@willingc
Copy link
Contributor

@ezio-melotti@hugovk Is this something that we can move forward? Or should it be closed?

@AA-Turner
Copy link
Member

The cited issues were with Sphinx 6 and earlier, which we have now dropped.

willingc reacted with thumbs up emoji

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. The keyword is looked up as an attribute on the subject.
I. The keyword is looked up as an attribute on the subject.

@@ -1120,13 +1120,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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
2. If all keyword patterns succeed, the class pattern succeeds.
II. If all keyword patterns succeed, the class pattern succeeds.


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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. The equivalent of ``getattr(cls, "__match_args__", ())`` is called.
I. The equivalent of ``getattr(cls, "__match_args__", ())`` is called.

Comment on lines +1147 to +1148
2. Once all positional patterns have been converted to keyword patterns,
the match proceeds as if there were only keyword patterns.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
2. Once all positional patterns have been converted to keyword patterns,
the match proceeds as if there were only keyword patterns.
II. Once all positional patterns have been converted to keyword patterns,
the match proceeds as if there were only keyword patterns.

Copy link
Contributor

@willingcwillingc left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks@ezio-melotti and@AA-Turner for moving this forward.

@hugovkhugovk removed the needs backport to 3.12only security fixes labelApr 10, 2025
@serhiy-storchakaserhiy-storchaka added the needs backport to 3.14bugs and security fixes labelMay 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@hugovkhugovkhugovk left review comments

@AA-TurnerAA-TurnerAA-Turner left review comments

@willingcwillingcwillingc approved these changes

@pradyunsgpradyunsgAwaiting requested review from pradyunsg

Assignees

@ezio-melottiezio-melotti

Labels
awaiting mergedocsDocumentation in the Doc dirneeds backport to 3.13bugs and security fixesneeds backport to 3.14bugs and security fixesskip news
Projects
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@ezio-melotti@willingc@AA-Turner@hugovk@serhiy-storchaka

[8]ページ先頭

©2009-2025 Movatter.jp