Use Autoformatting

[danieleades]

I feel fairly strongly that this repo should use some sort of pep8-compliant auto-formatter to enforce a common code formatting style.

There are loads of advantages to using an auto-formatter

• consistent visual style
• waste less time manually tweaking formatting
• whole class of flake8 lints are eradicated by the formatter
• never lose time discussing/debating formatting changes in a PR again

I don't have any strong feelings about which formatter should be used, or what configuration it should employ. I think the value is in the consistency, not the specific style. I've submitted a few PRs now where i've had to spend 10mins tweaking indents until flake8 is happy. That seems absolutely bonkers to me.

I have personally found Black to be really good and have previously suggested it in a rejected PR. I would welcome any other suggestions for a different pep8-compliant formatter

[tk0miya]

Note: #9955 (suggestion of black)

[AA-Turner]

Note also one of the first lines of PEP 8, that black does not follow in my opinion (though I recognise that's a tradeoff they make).

I've used autoformatters in other projects and there are absolutley pros and cons, I have had to manually edit statements to get it to format in a way that makes sense to me. However as you say it makes linting type issues easier.

A

[danieleades]

to be clear, black is pep8-compliant. It's code style is a superset of pep8.

[AA-Turner]

I'm not going to get into philisophical debates (life's too short!), my point was more against autoformatters in general -- consistency for the sake of it is (in my opinion) a bad goal.

If you can try YAPF or one of the configurable formatters that would have a very small diff on adoption, perhaps, though in general I'm somewhere between -0 and -0.5.

A

[danieleades]

I guess this is a philosophical point indeed- I think consistency for the sake of it is a deeply important goal. The larger the codebase, and the greater the number of contributors, the more valuable this becomes.

[picnixz]

@danieleades

Since I've started working on the testing plugin, I've been a bit frustrated some times due some rules of formatting, especially the 2 blank lines. Since you seem well-versed in all those tools, I'm asking some questions:

• is there a way to ignore the 2 blank lines rules when considering comments sections like

  def other(): ...
  # <blank>
  # <blank>
  #####
  # this is a section
  #####
  # <blank>
  # <blank>
  def function(): ...

• is there a way to allow 0-line when using @overload ? honestly, I think having blank lines around @overload definitions makes the code much more unreadable than helpful. Like

  @overload
  def foo(...): 
      ...
  # <blank>
  # <blank>
  @overloads
  def foo(...): 
      ...
  # <blank>
  # <blank>
  def foo(...): ...

  I would really like if we could have something like

  @overload
  def foo(...): ...
  @overloads
  def foo(...): ...
  def foo(...):
      ...

  without the linter complaining everytime without having to add # NoQA or 'fmt: off' everywhere. I can live with the '...' on a new line though but having the overloaded definitions physically far from the original definition makes 1) the file very big 2) is not helpeful when you read the code.

• If the above point is not possible, do you think it's good to have '.pyi' files at the level of our source code? or should we maintain a stub at the project's level?

• What were the reasons that we still need flake8?

[danieleades]

@danieleades

Since I've started working on the testing plugin, I've been a bit frustrated some times due some rules of formatting, especially the 2 blank lines. Since you seem well-versed in all those tools, I'm asking some questions:

I am emphatically not an expert in formatting code. I favour autoformatting precisely so that i never have to think about it, know about it, or have long bikeshedding conversations about it!

• is there a way to ignore the 2 blank lines rules when considering comments sections like

  def other(): ...
  # <blank>
  # <blank>
  #####
  # this is a section
  #####
  # <blank>
  # <blank>
  def function(): ...

i'm not sure

• is there a way to allow 0-line when using @overload ? honestly, I think having blank lines around @overload definitions makes the code much more unreadable than helpful. Like

  @overload
  def foo(...): 
      ...
  # <blank>
  # <blank>
  @overloads
  def foo(...): 
      ...
  # <blank>
  # <blank>
  def foo(...): ...

  I would really like if we could have something like

  @overload
  def foo(...): ...
  @overloads
  def foo(...): ...
  def foo(...):
      ...

you may wish to look at ruff's --preview formatting which formats these on one line as you're after

• If the above point is not possible, do you think it's good to have '.pyi' files at the level of our source code? or should we maintain a stub at the project's level?

is there a reason to have pyi files in sphinx at all?

• What were the reasons that we still need flake8?

you'd have to check with @AA-Turner. I think there's one rule that one of the flake8 plugins supports that has not yet been implemented in ruff

[picnixz]

is there a reason to have pyi files in sphinx at all?

A big reason is that we could move the very very big overloads to those files, and we wouldn't need to bother with the same format considerations (for instance, there are some parts of the code that are typed but where adding overloads for completion would be useful, though it comes at the cost of having a much larger file).

[danieleades]

@picnixz see astral-sh/ruff#8357

seems like ruff format will do what you're wanting

[danieleades]

is there a reason to have pyi files in sphinx at all?

A big reason is that we could move the very very big overloads to those files, and we wouldn't need to bother with the same format considerations (for instance, there are some parts of the code that are typed but where adding overloads for completion would be useful, though it comes at the cost of having a much larger file).

i'm very sceptical of out-of-source type annotations, but maybe i just need to see a motivating example

[picnixz]

Nah I think that if you are not convinced it's probably an overkill then, nevermind!

[chrisjsewell]

@AA-Turner given #12335 and #12337 I would like to revisit this, and just make a final decision either way 😅

From my understanding, I believe many active maintainers are in favor of this: me, @picnixz, @danieleades, @jayaddison, ...
but obviously I would want them to speak for themselves 😄

ruff format is essentially the de-facto standard for this now; I don't see that changing anytime soon.
I use it for all my projects, including myst-parser, and have never had any issues.

We have already gone through a round of applying this to smaller sections of the code-base, and working through our understanding of the formatter and how to minimise diffs:

• 🔧 Ruff format python files within docs folder #12139
• 🔧 Ruff format python files within utils folder #12142

[jayaddison]

I'm cautiously in favour of doing this, but perhaps we should delay for a while (a month?) to ensure that any further 7.3.0 issues are tracked down before doing something that could complicate code history trawling? The .git-blame-ignore-revs file does help with that, but it isn't entirely quirk-free (git may indicate that some lines modified by an ignored rev were related to the previous commits for surrounding lines, for example).

Also a question that I couldn't find a clear answer to: is there a standard way to annotate package metadata to say what (if any) Python code formatter (incl. version) was used for the code in a distributed package? Probably not at the moment I guess.

[danieleades]

Also a question that I couldn't find a clear answer to: is there a standard way to annotate package metadata to say what (if any) Python code formatter (incl. version) was used for the code in a distributed package? Probably not at the moment I guess.

What would be the application for that?

[chrisjsewell]

I'm cautiously in favour of doing this, but perhaps we should delay for a while (a month?) to ensure that any further 7.3.0 issues are tracked down before doing something that could complicate code history trawling?

To be clear, I'm not even saying we format all the code straight away; just to continue along the road of gradually removing files from the ruff.format.exclude list

[jayaddison]

Also a question that I couldn't find a clear answer to: is there a standard way to annotate package metadata to say what (if any) Python code formatter (incl. version) was used for the code in a distributed package? Probably not at the moment I guess.

What would be the application for that?

@danieleades it's a fairly obscure idea, but here it is: some distributions that package Python code include the test suite alongside, so that the distros themselves and/or their users can acceptance-test the contents. However, the source code of a Python package doesn't typically contain any reference (not in the manifest, nor elsewhere) of any linting enforcement applied to the codebase. Linting seems unlikely to currently be in the acceptance criteria for most distributed Python packages, but a suitable metadata entry could enable that possibility.

[picnixz]

I personally have shortcuts for autoformatting now that ruff format is enabled and it does pretty much what I want. The only exception is about the way a call is being formatted or when nested lists are being unfolded, e.g.:

@pytest.mark.parametrize(('maxsize', 'start', 'count'), [
    # combinations of integers (a, b, c) such that c >= 1 and a >= b + c
    (1, 0, 1),
    (2, 0, 1), (2, 0, 2), (2, 1, 1),
    (3, 0, 1), (3, 0, 2), (3, 0, 3), (3, 1, 1), (3, 1, 2), (3, 2, 1),
])  # fmt: skip

would become

@pytest.mark.parametrize(
    ('maxsize', 'start', 'count'),
    [
        # combinations of integers (a, b, c) such that c >= 1 and a >= b + c
        (1, 0, 1),
        (2, 0, 1),
        (2, 0, 2),
        (2, 1, 1),
        (3, 0, 1),
        (3, 0, 2),
        (3, 0, 3),
        (3, 1, 1),
        (3, 1, 2),
        (3, 2, 1),
    ],
)

and this is ONE thing that I hate because I really want to separate the cases (and for me the first form is much clearer than the second one). So, I use # fmt: skip for those cases and ruff doesn't bother me anymore.

Since we have a CI check that fails if ruff format fails, I think we can just add the auto-formatting for PRs by using pre-commit. I'm not sure whether you can detect the bad cases I just mentioned before actually committing and that's one thing I don't know how to fix (I prefer invoking the formatter myself and then I commit if I'm happy with the results).

Now, I think it's fine to autoformat files that are already present and yet to be formatted. But I'm not sure how it would be for opened PRs (it usually creates conflicts...). By the way, when I was doing the refactorization of the typing module, the autoformat messed up mypy ignores so it's also not a perfect solution and should be carefully done.