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

autoformat using black and isort #9955

Closed
wants to merge 2 commits into from
Closed

autoformat using black and isort #9955

wants to merge 2 commits into from

Conversation

danieleades
Copy link
Contributor

Is this repo autofomatted? should it be?

@danieleades
Copy link
Contributor Author

(if yes, can I suggest extending this PR to add autoformat pre-commit hooks and ensure it stays formatted?)

@tk0miya
Copy link
Member

tk0miya commented Dec 10, 2021

If my memory is correct, maintainers discussed using black on this project and rejected the proposal at that time. And I also don't prefer it. So -1 for merging this.

@danieleades
Copy link
Contributor Author

If my memory is correct, maintainers discussed using black on this project and rejected the proposal at that time. And I also don't prefer it. So -1 for merging this.

can you be more specific?

  • are there particular choices in black that you don't like?
  • is there specific configuration other than the default that you prefer? for example a longer or shorter line length?
  • is there another pep8-compliant formatter that you prefer to black or do you prefer using exclusively manual formatting?

I think there's a lot of advantages to auto-formatting in python. For one, once you decide a format style, you never have to talk about formatting again. Also code is easier to grok when it's consistently formatted. I don't have shares in Black or anything, I don't much care what the format is. It's the consistency that's useful. Although Black has the distinct advantage of being very unconfigurable. That saves a lot of bike-shedding.

I think the most compelling use for this codebase is the complex, nested if blocks that seem to be ubiquitous in sphinx. manually formatting and aligning multi-line conditionals is utter madness.

@tk0miya
Copy link
Member

tk0miya commented Jan 9, 2022

I know auto-formatting is very worthy in my head. But I can’t have a positive feeling for the format of black very much. So I'd not like to vote for this. Please ask to other maintainers or join maintainers. I won't opposite to merge this if they agreed.

Note: This change will make existing pull requests not mergeable.

@danieleades
Copy link
Contributor Author

Note: This change will make existing pull requests not mergeable.

This is true. Unless there are any large PRs expected to be merged soon, there's probably no better or worse time to rip the band-aid off.

With regard to Black's style- I'm very very strongly in favour of auto-formatting in general, and especially in large open-source projects where consistent formatting is hugely valuable. I'm not married to Black's style in particular (though it happens I do like it).

The advantage of black is the lack of configurability. It completely eliminates the need for discussion and debate around formatting. That's maybe a divisive design choice...

I'd be happy to consider alternative suggestions for pep8-compliant formatters.

@jakobandersen
Copy link
Contributor

I'm not against using Black in principle, but in practice I there are problems with a sweeping auto-format.

  1. The breaking of existing PRs is extremely problematic. I bet most people have limited time to contribute to Sphinx, at least I do. Who will update all the broken PRs?
    I also have branches that are not ready for PRs, and I really don't want to spend extra time updating them.
  2. I briefly scanned som of the parts I usually work on (sphinx/domains/{c,cpp}.py) and some of the formatting I either find less readable or outright more confusing. E.g., the module-level lists. They are manually formatted to group semantically similar things.
    Another example is the idea of formatting function signatures on a single line instead of, e.g., 2 lines with a hanging indent (see the describe_signature functions). It removes hints on the semantic meaning of the parameters. The other signature formatting of every parameter on its own line is also problematic (e.g., see ASTArray.__init__) as it again removes semantic hints, and makes the code less readable for me. Generally I find it much more readable with hanging indents, unless a single parameter (or argument for function calls) will overflow a line.

Is there a strategy to incrementally auto-format the code base over time instead?
Is there a way to disable certain types of formatting cases on a per file basis?

@danieleades
Copy link
Contributor Author

I'm not against using Black in principle, but in practice I there are problems with a sweeping auto-format.

  1. The breaking of existing PRs is extremely problematic. I bet most people have limited time to contribute to Sphinx, at least I do. Who will update all the broken PRs?

I don't see a way around that, unfortunately. I think it's worth the pain though, to be honest. it also means you won't get merge errors due to formatting changes again going forward.

I also have branches that are not ready for PRs, and I really don't want to spend extra time updating them.

sweeping formatting changes will always cause problems for open PRs, but we can hold off until specific PRs are merged, if that helps.

  1. I briefly scanned som of the parts I usually work on (sphinx/domains/{c,cpp}.py) and some of the formatting I either find less readable or outright more confusing. E.g., the module-level lists. They are manually formatted to group semantically similar things.

note that this PR has both black and isort changes. I can split out isort and that'll leave the import ordering alone. Would that help? I can also exclude specific files from isort's attention.

Another example is the idea of formatting function signatures on a single line instead of, e.g., 2 lines with a hanging indent (see the describe_signature functions). It removes hints on the semantic meaning of the parameters. The other signature formatting of every parameter on its own line is also problematic (e.g., see ASTArray.__init__) as it again removes semantic hints, and makes the code less readable for me. Generally I find it much more readable with hanging indents, unless a single parameter (or argument for function calls) will overflow a line.

can you add some inline comments where you see issues? Might be easier to discuss in context

Is there a strategy to incrementally auto-format the code base over time instead? Is there a way to disable certain types of formatting cases on a per file basis?

yes. It's absolutely possible to exclude specific files or directories

@tk0miya tk0miya changed the base branch from 4.x to 5.x May 22, 2022 12:56
@AA-Turner
Copy link
Member

Two maintainers have spoken against this PR (right now) -- closing, but we might revisit the issue later. We already use isort.

A

@AA-Turner AA-Turner closed this May 23, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 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

4 participants