• Ensure binary operators are surrounded by spaces

What is your basis for that style rule? Is this something that black does? PEP 8 does not require spaces around all binary operators; see https://peps.python.org/pep-0008/#other-recommendations. The example x = x * 2 - 1 in that section, with spaces around all the binary operators, is explicitly stated to be wrong. I think most of your changes look fine, except that expressions like 0*b and 1.0/diagOfA should be considered acceptable.

We should avoid style changes that are based on personal preferences rather than explicit guidelines. PEP 8 does say "use your own judgment", but without a clear set of style guidelines to follow, the next person to do some clean up might actually undo some of your changes, and we can end up with unproductive style churn.

Ah thank you so much for this. Whenever you feel like done, let's get this in. I don't want you to run out of steam while you are having valuable time :) Then I'll rebase.

x = x * 2 - 1

For this I'd say x*2 - 1 is good grouping compared to x*2-1 or x * 2 - 1 since power belongs to x semantically. Maybe it's the fonts push this but all cramped or all separated is not looking good at all.

But since this is a pre-PR for #18391 let's not waste time on such details. I can fix it over there, all the danglings we missed here.

let's not waste time on such details.

Maintaining consistent coding style is not a waste of time. In this case, my suggestion should avoid wasting future work by authors and reviewers in making style changes that might be based on personal preferences instead of established guidelines. Also, the best time to get code correct is in the original pull request. Unless there is a really compelling reason, it is better to fix it before it gets into main instead having to fix or undo changes in another pull request later.

I won't block this or comment further, but I hope I've at least raised awareness about what PEP 8 does and doesn't say, and about avoiding style changes that are based on personal preferences instead of established guidelines.

Every time style issues like this come up, I'm even more convinced that we should go ahead and adopt black and be done with style discussions and clean up forever.

What is your basis for that style rule? Is this something that black does?

Yes, it's inspired directly by black, which - while I don't necessarily agree with the outcomes in all cases - is just such a boon by avoiding (for codebases that adopt it) all these formatting discussions. I guess I have gotten used to the less crammed style as well.

As you might infer from the above, I have no interest in litigating style choices, and will happily follow whatever people feel strongly about.

Every time style issues like this come up, I'm even more convinced that we should go ahead and adopt black and be done with style discussions and clean up forever.

100%

I think this doesn't get to any of the subtleties mentioned in the PEP8 section (which needs at least two operations of different precedence; if only visually). I tried to look for examples but couldn't find any.

I think most of your changes look fine, except that expressions like 0*b and 1.0/diagOfA should be considered acceptable.

In this case do think that 0 * b is marginally better than 0*b and likewise for the division. But if you feel strongly about this, I'll happily revert those (and other occurrences, if you point them out to me).

@WarrenWeckesser I am jumping on your last comment about adopting black 😉 I am happy to propose this again. We did not really make a decision last time.

I don't think the issue here is warranting a black adoption so let's not carried away here. This discussion is definitely not happening all the time plus I didn't make a controversial statement I hope.

What I mean to say is that binary operator or any other preference is fine for me. I do not really want to specify things down to that level which is nothing but nitpicks and probably that's why PEP is letting it loose to which I agree wholeheartedly.

Another point about this particular code style thing, the whole codebase is currently being ignored with completely non-PEP8 py files and the moment we touch them it becomes a review burden. So if we want to be consistent I can definitely send a 150 file fix like we did with linting everything. That would be my preference, but we cannot both say we don't want stylistic changes in separate PR and also split hair on functional PRs.

We need a bit of change of mentality here.

We need a bit of change of mentality here.

I don't understand what you mean to say here.

For the rest, we keep having PEP and styling discussions, like all the time, in reviews.

For the rest, we keep having PEP and styling discussions, like all the time, in reviews.

I don't know if keep having them. It's mostly "nitpick but maybe do this, that" level. I'm not following everything lately but at work I know what "discussion" means. I've been in 2 hour meetings about what black did and did not. So we don't have even remotely close to an actual style discussion/war. Most of the black related discussion doesn't even apply to our codebase anyways. No API calls no dependency injections none of that "foreign" code problems apply to us. So in my opinion, bunch of plus signs and linebreaks do not grant a full blown strategy change, unless you really choose to be bothered by it

But then our policy is

• don't send PRs about style changes.
• OK why?
• Because git commit history.
• What's about it?
• It clutters the git blame.
• OK when do we do it?
• When we touch the files.

But when we touch the files, we have other concerns, then just like above we have this weird thing "we should separate the pep8 to a different PR" yes but then why don't we do it properly all at once?

This policy is effectively dragging us behind that I don't want to touch any file. Just like the original files for the FORTRAN related things. We need proactive handling of these issues not bury it until next decade.

But when we touch the files, we have other concerns, then just like above we have this weird thing "we should separate the pep8 to a different PR" yes but then why don't we do it properly all at once?

In this case, this was my "fault", but I'm going to stand by the principle. Doing a deep refactor like #18391 crucially relies on our tests covering the required API/behaviour, and as a reviewer (and future git archeologist), I do not want to have to double-check in such cases that the huge diff I see for the tests are non-functional, cosmetic changes.

I don't care so much when or how we do stylistic clean-ups, except that it should IMO absolutely not be mixed with big PRs that rely on the tests being (essentially) unchanged.

I have been having a close look at every single PR and issue for the last 2 years at least, I can tell that we keep having such debate (nitpick, discussion, etc. a lot of us are non native speakers and put a different weight on words) over and over.

It seems like you would be ok if we were to do a massive change at once. This is my preference as well, and since black is the norm, I would go with it (I even had a PR doing that.) To me, being perfect or not is besides the point of it being a standard.

We can keep doing nothing and never talk again about styling, newcomers are still going to use black (as observed already in a lot of PRs from newcomers here) and our style will slowly move towards black's styling invariably as new code get's in.

Yes but the reason why people are resisting is because you are advocating black coding style even with a precommit hook. Coding style is not equate to PEP8 compliancy. Nobody is resisting to a PEP8 compliancy whether it is black output or somebody hand-tunes it. The idea is PEP8 compliancy not how we get there.

That's where things get controversial because that's an opinionated code and I have some preference, you have another. As long as we don't ask people to use black, I don't think we will get any domination from black. But not that I really care.

If it is ok for pep8 I want to be done and I would like to keep things readable with sufficient common-sense in their own context and not because some tool decides for me. So running black on the whole codebase is just going to ruin some context and we wouldn't know until we really need that context. There are much less intrusive tools that only do obvious fixes and leaves the rest as warnings. Takes a bit more manual effort but the result is way less controversial.

In this case, this was my "fault", but I'm going to stand by the principle. Doing a deep refactor like #18391 crucially relies on our tests covering the required API/behaviour, and as a reviewer (and future git archeologist), I do not want to have to double-check in such cases that the huge diff I see for the tests are non-functional, cosmetic changes.

You can't do that unless you are really doing it after the fact. So that purism that i really enjoy myself too, is not available all the time. So unfortunately, that's a principle but in an idealistic one and not a practical one. It is exceptionally messy to get things working and you must come back to certain changes as you encounter them.

You can't do that unless you are really doing it after the fact.

I think we might still be talking about different things. This PR here (+ #18463) is my claim & proof that most of the changes to scipy/sparse/linalg/_isolve/tests/test_iterative.py in #18391 can be done independently & before-hand (though not 100% of changes of course; that's not realistic and I never meant to imply that).

Yes but the reason why people are resisting is because you are advocating black coding style even with a precommit hook. Coding style is not equate to PEP8 compliancy. Nobody is resisting to a PEP8 compliancy whether it is black output or somebody hand-tunes it. The idea is PEP8 compliancy not how we get there.

I honestly don't get why people are resisting... (from a technical perspective and see bellow)

That's where things get controversial because that's an opinionated code and I have some preference, you have another. As long as we don't ask people to use black, I don't think we will get any domination from black. But not that I really care.

That's my point, our personal preference should not matter for such project. If we want the code to be maintainable by anyone, it should not be "tainted" by any project's nor person's specific styling. It's already hard to contribute to SciPy, having some extra coding preferences to learn (vs running an automated tool) is definitely adding to the churn (it is in practice, a lot of newcomers are beginners with little knowledge.)

If it is ok for pep8 I want to be done and I would like to keep things readable with sufficient common-sense in their own context and not because some tool decides for me. So running black on the whole codebase is just going to ruin some context and we wouldn't know until we really need that context. There are much less intrusive tools that only do obvious fixes and leaves the rest as warnings. Takes a bit more manual effort but the result is way less controversial.

There should be no context with styling. If that's the case, it would mean only a certain group of maintainers are able to get/do something. It's neither inclusive, nor maintainable and wanted in terms of "bus factor."

As the time goes, more and more projects have migrated now to black. We really start to be outliers here. (We definitely are for pre-commit.)

Can we move the general formatting discussion into a separate issue? It think it's quite unrelated to this PR by now.

Sure sorry about that 🙇

I honestly don't get why people are resisting...

That seems to be the problem. And possibly you don't want to move your stance either so each time we have to have the same discussion. I don't like black output but I am pep8 compliant so that should settle the issue but somehow we are continuously pushed to black and we don't need it. Like I said, I can do this manually for the whole codebase in 15 minutes too. I don't need black. Certainly not as a precommit hook.

But in general, I cannot spare time each time I'm doing a refactoring, oh let me open a pre-PR so that we can start working. That will not fly, at least I cannot be that organized and we will have this issue. That's what I meant by our self-conflicting policy.

Here this unitary operator thing started it. And that's exactly the issue. As long as we accept that this is still a preference I'm fine. But you should also register this fact that it is a preference. Git commit history is not a valid argument for such a big refactor.

I am not even sure if the code is working fine on the original PR, but instead we are dwindling into unnecessary styling issues which is a result of our preference OCDs kicking in while half of the entire codebase is painted red on my editor. That is my issue, not black or whatever.

And possibly you don't want to move your stance either so each time we have to have the same discussion. I don't like black output but I am pep8 compliant so that should settle the issue but somehow we are continuously pushed to black and we don't need it.

FWIW, I fully agree with @tupui on this. Your (or anyone else's) personal preference does not outweigh the benefits that black brings at scale - not just in terms of automatability, uniformity (not only within scipy but also with a large number of other projects) etc., but far and above anything else: in terms of the time saved from never having to discuss formatting again (including which warnings to enable or disable with some PEP8-based tool).

But, as I said, let's please take this discussion elsewhere.

Can we move the general formatting discussion into a separate issue? It think it's quite unrelated to this PR by now.

Agree, despite the big-picture discussion I think we should hit the green button on this to keep the #18391 ball rolling

Now that it is merged, I am going full disagree with this I have to say. You started you finished this discussion about styling so it is a self fulfilling prophecy. I am trying to fix a PR and getting style discussion by you and then I am being the pusher of my preference.

I have to object that it is your preference that you started this here. So let's not ignore this, which is often the case when people defend black. So unfortunately I won't take this argument serious in this condition. Like I said, I'm full in for PEP8 compliancy but that's where it ends for me. Not PEP8 compliancy with a tool and a codestyle pushed in the name of it.

And I am reiterating, we don't have any policy about bunch of plus signs. We are not creating a million SLOC finance application with custom classes. They are at most classes of numerical operations.

Now that it is merged, I am going full disagree with this I have to say. You started you finished this discussion about styling so it is a self fulfilling prophecy.

For context: I asked to separate your style changes from #18391. You said it cannot be done, and rather than belabour the point, I just went ahead and made a PR myself, based on your style changes, though I cleaned up related inconsistencies that stood out to me (you had added spaces around some operators, not others). At no point did I say my changes are the final word, and repeatedly offered to back out anything controversial.

I'm not attached to black, what I'm attached to is not having exactly this kind of conversation. If you have an alternative with comparable benefits, and care to convince everyone to go with that, please go ahead.

@ilayn, I made the initial comment questioning some of the style changes in this PR. Was your last comment directed at me? I said I wouldn't comment further, but the PR is now merged, so further discussion won't hold up the work you and @h-vetinari are doing.

@ilayn, I made the initial comment questioning some of the style changes in this PR. Was your last comment directed at me?

No, frequencies interfered. All good.

For context: I asked to separate your style changes from #18391. You said it cannot be done, and rather than belabour the point, I just went ahead and made a PR myself, based on your style changes, though I cleaned up related inconsistencies that stood out to me (you had added spaces around some operators, not others).

Yes, and I didn't raise any objections to any style changes myself. But anytime the minute comment comes in about a stylistic choice we end up with black. That is getting really tiring.

I don't know how many more times we have to perform surgery on this. It is an enforcer not unifier. And that point is always getting lost in any nuance. I am fine with whatever choice you pick. You can even pass it through black yourself. I don't care probably others don't either. I am OK with whatever path you picked to end up with PEP8 compliant code.

The point I, and many others before, objecting to is enforcing black pass as mandatory. I am not sure if I can make it more clearer more than this.

The problem with leaving the door open to anything is that we keep having discussions. In general, that's simply an expression of our governance structure. A few folks are very vocal about a style or not, black, or one of its variation. To me, this is far from being inclusive (we very rarely have more than a handful of maintainers deciding) nor representative of the current needs (in terms of maintenance of the project and efforts needed from active maintainers.)

You might not care about black or which style is used. I argue that you are senior enough and a good developer enough to be able to make a sensible decision. Again, this is far from being newcomers friendly. We invariably have to teach newcomers about style, and that's a fact.

So yes, I am sorry that I and some other maintainers keep coming back with black. This is a real need, and, on a personal note, would make my life/job as a "contributor experience lead of SciPy" way easier.

It's perfectly fine to tell contributors, if you prefer and familiar with it, use black or any other PEP8 compliancy tool (with common sense) instead of mounting a pre-commit hook. Why is it difficult to convey the message that the enforcing of black is the problem not black itself.

Simply because all tools are not compatible. Opinionated tools would want to change the files style differently and you could end up adjusting the style over and over. We are seeing this behavior already and we often have to ask people to revert some changes they do to a whole file.