🔧 Ruff format select sphinx modules with minimal diffs #12146

chrisjsewell · 2024-03-19T19:15:46Z

This PR removes select sphinx top-level modules from thr ruff format exclude list.
They were selected based on the fact that they have the least diffs when formatted, as hopefully you can see.
I don't believe these changes should adversely affect any existing PRs.

jayaddison · 2024-03-19T19:29:17Z

Any plans to include some/all of these changesets in .git-blame-ignore-revs?

I'm supportive of the styling changes, but I spend a lot of time in git blame view :)

(note: there could be odd side-effects from that file, like making it difficult to intentionally track down when style-related changes occurred)

chrisjsewell · 2024-03-19T19:30:42Z

Any plans to include some/all of these changesets in .git-blame-ignore-revs?

I've never used this really before, so I'm open to suggestions

jayaddison · 2024-03-19T19:31:55Z

Ok. Don't let it block any progress -- we can't use it until we know the merged git commit IDs anyway -- but perhaps worth keeping in mind.

jayaddison · 2024-03-19T19:33:29Z

sphinx/extension.py

-            logger.warning(__('The %s extension is required by needs_extensions settings, '
-                              'but it is not loaded.'), extname)
+            logger.warning(
+                __(
+                    'The %s extension is required by needs_extensions settings, '
+                    'but it is not loaded.'
+                ),
+                extname,
+            )


Maybe a manual tweak here: would it look better to extract a msg variable for the localized warning message?

but the msg still does not fit on one line, without going over the line-length limit, so it will not change the number of lines

Possible I missed something, but this works for me with ruff locally:

- logger.warning( - __( - 'The %s extension is required by needs_extensions settings, ' - 'but it is not loaded.' - ), - extname, - ) + msg = __('The %s extension is required by needs_extensions settings, ' + 'but it is not loaded.') + logger.warning(msg)

ah, yes - the extname. That's important...

(even with extname added back in, it seems to pass ruff checks OK. I think it makes the overall diff marginally easier to comprehend, too, which is sometimes nice)

That is extremely strange that it passes on my machine. Ok, ignore my suggestion - I'll spend some time to figure out how that happened soon. Apologies for the distraction!

(and thanks for providing a CI-based confirmation, I should have thought to try that!)

no worries cheers

Ok; I'm pausing for a moment just in case this is relevant.

When using the full file path (ruff check sphinx/extension.py) it seems that our .ruff.toml config is ignored.

That's why this passed when I ran the command above.

When I run ruff check in the root sphinx.git directory with my suggestion in place, it fails (F821).

I think that means that something in our project configuration (either flake8 or ruff related) introduces this failure. I'd like to confirm what that is.

Ok, confirmed: it is due to our config choice here:

sphinx/.ruff.toml

Lines 116 to 117 in 392358d

# pyflakes ('F')

"F",

jayaddison · 2024-03-19T19:35:34Z

sphinx/theming.py

-                    logger.warning(__('file %r on theme path is not a valid '
-                                      'zipfile or contains no theme'), entry)
+                    logger.warning(
+                        __(
+                            'file %r on theme path is not a valid '
+                            'zipfile or contains no theme'
+                        ),
+                        entry,
+                    )


Ditto here re: possible variable extraction. Consistent styling will be great; 2 lines compared to 7 seems a significant difference, though!

same as above, the msg won't fit on a single line, so it seems not worth the effort to extract out a msg variable (you'll still have the same number of lines)

picnixz · 2024-03-20T11:17:07Z

sphinx/jinja2glue.py

            template = template[1:]
        for loader in loaders:
            try:
                return loader.get_source(environment, template)
            except TemplateNotFound:
                pass
-        msg = (f"{template!r} not found in "
-               f"{self.environment.loader.pathchain}")  # type: ignore[union-attr]
+        pathchain = f'{self.environment.loader.pathchain}'  # type: ignore[union-attr]


This f-string is useless.

Sure, I had to move this to a seperate variable only because of flake8: ruff actually allows for leeway in the line-length, if there is a comment making it longer, but flake8 does not

i suggest suppressing the flak8 lint

i suggest suppressing the flak8 lint

Ok I just increased the flake8 line-length to 120, and then put all this back on one line

sphinx/jinja2glue.py

sphinx/locale/__init__.py

sphinx/events.py

picnixz · 2024-03-20T11:23:31Z

sphinx/deprecation.py

-        message = (f'The alias {qualified_name!r} is deprecated, '
-                   f'use {canonical_name!r} instead.')
+        message = (
+            f'The alias {qualified_name!r} is deprecated, ' f'use {canonical_name!r} instead.'


This one is confusing in the sense that two f-strings are concatenated on the same line.

Is there a way for ruff to merge the f-strings when possible?

fixed manually

Is there a way for ruff to merge the f-strings when possible?

It does not appear so, maybe a good feature request to open on ruff 😄

sphinx/roles.py

sphinx/extension.py

sphinx/theming.py

picnixz

Except for the bad f-strings that were introduced, I think I'll need to live with this.

chrisjsewell · 2024-03-20T13:42:08Z

sphinx/extension.py

+            msg = __('The %s extension is required by needs_extensions settings, '
+                     'but it is not loaded.')
+            logger.warning(msg, extname)


@jayaddison would you confirm this is how you proposed formatting it?

and then can you also see, this fails the CI checks:

I feel something in your set up is wrong 😅

chrisjsewell · 2024-03-20T13:45:58Z

Except for the bad f-strings that were introduced, I think I'll need to live with this.

thanks for the review, obviously a few of my answers "if you have a problem, take it up with ruff" 😄 ,
but its still good to have a look and see how these things work and understand where there might be diffs

picnixz · 2024-03-20T14:39:55Z

"if you have a problem, take it up with ruff" 😄

Well.. that's usually what I do when I want something upstream. It's just I'll need to change my PyCharm's bindings to avoid bad autoformats (but that's something for me only).

chrisjsewell · 2024-03-20T14:41:29Z

It's just I'll need to change my PyCharm's bindings to avoid bad autoformats

It doesn't have ruff integration?

jayaddison · 2024-03-20T15:07:29Z

.flake8

@@ -1,7 +1,8 @@
 [flake8]
-max-line-length = 95
+max-line-length = 120


Let's update this in ruff.toml (and anywhere else?) too.

In fact: I think ruff will read it from flake8's config if it can. So we should remove it from ruff.toml, I think.

no, this is just to stop flake8 complaining in places ruff does do not; ruff allows leniency in line-length for lines with comments, flake8 does not

we are just deferring the line-length checks to ruff, obviously eventually we will hopefully be removing flake8 entirely

In fact: I think ruff will read it from flake8's config if it can. So we should remove it from ruff.toml, I think.

No, I may be talking rubbish unfortunately. I was distracted by the fact that ruff config line-length mentions 120, which is, by chance, what we're updating flake8 to use here. But it doesn't confirm that ruff is reading that value.

i would actually suggest suppressing the lint in flake8 entirely, rather than just making the line length limit greater

how do you do that?
(I'm not that bothered either way, because I hope we just get rid of flake8 sooner rather than later 😄 )

i would actually suggest suppressing the lint in flake8 entirely, rather than just making the line length limit greater

Only for comments, ideally?

max-doc-length could achieve that I think, if so.

how do you do that?

add E501 to the 'ignore' list in the flake8 config file

Only for comments, ideally?

ruff ignores comments when calculating line length, flake8 does not.
ruff allows longer lines in a few select cases:

Overlong lines can hurt readability. PEP 8, for example, recommends limiting lines to 79 characters. By default, this rule enforces a limit of 88 characters for compatibility with Black, though that limit is configurable via the line-length setting.

In the interest of pragmatism, this rule makes a few exceptions when determining whether a line is overlong. Namely, it:

Ignores lines that consist of a single "word" (i.e., without any whitespace between its characters).
Ignores lines that end with a URL, as long as the URL starts before the line-length threshold.
Ignores line that end with a pragma comment (e.g., # type: ignore or # noqa), as long as the pragma comment starts before the line-length threshold. That is, a line will not be flagged as overlong if a pragma comment causes it to exceed the line length. (This behavior aligns with that of the Ruff formatter.)

add E501 to the 'ignore' list in the flake8 config file

done 👍

🔧 Ruff format sphinx modules with minimal diffs

6ecb9ad

chrisjsewell requested review from danieleades and picnixz March 19, 2024 19:15

Update jinja2glue.py

c26244f

jayaddison reviewed Mar 19, 2024

View reviewed changes

danieleades approved these changes Mar 19, 2024

View reviewed changes

picnixz reviewed Mar 20, 2024

View reviewed changes

picnixz self-requested a review March 20, 2024 12:40

picnixz approved these changes Mar 20, 2024

View reviewed changes

chrisjsewell added 2 commits March 20, 2024 14:34

Add review suggestions

c64ad3a

try @jayaddison format

8fc8801

danieleades approved these changes Mar 20, 2024

View reviewed changes

chrisjsewell commented Mar 20, 2024

View reviewed changes

chrisjsewell requested a review from jayaddison March 20, 2024 14:50

revert change

e576d03

jayaddison reviewed Mar 20, 2024

View reviewed changes

Update .flake8

626194b

jayaddison mentioned this pull request Mar 20, 2024

[tests] utils: refactor type-hint signatures. #12144

Merged

chrisjsewell merged commit 2d6f73e into sphinx-doc:master Mar 20, 2024
22 checks passed

chrisjsewell deleted the ruff-format-some-modules branch March 20, 2024 20:28

jayaddison mentioned this pull request Mar 21, 2024

[refactor] test utils: simplify signatures and remove from ruff exclusion list. #12155

Merged

github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2024

AA-Turner added this to the 7.3.0 milestone Jul 13, 2024

🔧 Ruff format select sphinx modules with minimal diffs #12146

🔧 Ruff format select sphinx modules with minimal diffs #12146

Conversation

chrisjsewell commented Mar 19, 2024

jayaddison commented Mar 19, 2024

chrisjsewell commented Mar 19, 2024

jayaddison commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell commented Mar 20, 2024

picnixz commented Mar 20, 2024

chrisjsewell commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Mar 19, 2024 •

edited

Loading

chrisjsewell Mar 20, 2024 •

edited

Loading

picnixz left a comment •

edited

Loading