Add black linting of tests/functional #10299
Conversation
|
I’m assuming this can only be merged once we’ve formatted the files. |
|
Okay, there's several issues I have with the approach taken here:
I'm going to close all the existing PRs, that were made in an automated manner. Unless we fix the first issue, all of this effort is likely going to go to waste and cause more pain. |
|
#10281 seems like the original PR that lead to the breaking up. I couldn't even locate that given the notification spam until today morning (I spent 20 minutes clearing that up). There's quite a bit of discussion there while I was out-of-action, but there's some things that I want to say. We decided to enable black incrementally on the codebase, since folks wanted to triage every single line that black spews out. This is the exact opposite of what Black recommends. We're horribly messing up our git blame in the process, and we've wasted many hundreds of CI hours on this already. I feel like I've not made it clear that I strongly dislike this. While, yes, we're adopting black -- we're also doing it in the most painful way possible to accomodate for "I want to review every line". If one of the maintainers really wants to review black's changes line-by-line, please say so explicitly here. I'd especially like to hear from @pfmoore and @uranusjr on this. If no one is interested, we don't have to go through a painful adoption story. I'd appreciate if you could provide a concrete reason/concerns so that I can check if we can solve that concern with a one-time linter run+manual fixes (eg: with https://pypi.org/project/flake8-implicit-str-concat/). I really want to make sure that we're not inflicting additional pain on ourselves, just because we didn't communicate about what-we-want clearly. Doing it in one go is an order of maginitude less work and pain for all of us. If no one says they want to review formatting changes line-by-line by... 19th Aug (7 days, including today), I'll be making a single PR on 20th, that completely reformats the codebase, does all the string concatenation shenenigans and all that jazz. I do genuinely appreciate the interest @nipunn1313 has shown here and I don't want to discourage them -- we could definitely use more hands and help here! That said, filing double digit PRs automatically without getting confirmation that each individual PR was complete was... not-great. I know that every maintainer involved in the original discussion was surprised by the automated PRs too. IMO the PRs were all missing the most important parts of the migration -- enabling CI checks on those files and doing bundles of not-too-many files at a time to minimise PR count. I was especially surprised (and annoyed) since I've been trying to get this stuff going without getting anyone more worked up about this, and threading the social + technical concerns here. I did that by setting boundaries on how I'd do this. The automated PRs have violated nearly all of the boundaries I'd set for the adoption. To name the obvious ones that I'm comfortable stating publicly:
Arguably, this is partly on me -- as the person trying to drive the adoption of black in this codebase -- since I hadn't clarified these earlier anywhere a new contributor could've learnt them outside of asking me. I thought given that I'm driving the changes, if a new contributor is interested, I'd have the opportunity to let them know about these things. Until the day #10281 was filed, the only person who needed to know all this context was me. Combined with the perfect overlap with my second CoVID-19 jab that took me out-of-action for a few days, that folks didn't wait on my inputs, and all the other communication gaps that occured... here we are. :) I think it's fairly obvious that no individual person did what they thought was appropriate and, given that I never shared the context I'd built up, we've ended up doing something that's suboptimal and... well, I'd like to cut us off before we continue that way further and cause more pain. Apologies for this mess! Hopefully, no one comes away feeling too frustrated/annoyed here. |
FWIW, I think it's kinda amusing that the first PR to be affected by this is #9086 (a really useful change by @nipunn1313). It's got merge conflicts now due to the reformatting, and fixing them the "wrong way" would have the CI pass, but the code wouldn't be formatted with black; leading to more work when we actually enable the CI checks on those files. |
|
To respond to @pradyunsg's specific question, I do not intend to review the black changes line by line. I assume (hope) that you will be doing that as part of implementing the changes but I don't think it needs more review than that. Specifically though @pradyunsg, please make sure we don't end up with any of those I would however like to add some personal context here (@pradyunsg please note this is a personal viewpoint and I do not intend to question or block the migration that I know you are strongly in favour of, nor do I want to imply that I think your work is unappreciated or wasted). For me, this is precisely the sort of frustrating "social" issue that the adoption of black triggers, it's a controversial choice that people end up disagreeing over and debating, resulting in confusion and unnecessary conflict. The particular irony to me is the fact that the disagreements are entirely about changes that are purely formatting changes - precisely the type of thing that black is explicitly intended to stop 🙁 Add to that the fact that on pip, I don't recall ever actually having a debate about actual code formatting that wasn't trivially resolved prior to the discussion about adopting black. I will ultimately "learn to live with" black, but it does not fit well in my development workflow (specifically, the fact that it has to run after the fact on a complete saved file - as far as I know there's no "apply black as you type" option, short of some sort of grand "save a copy, apply black, diff and parse the diff" hack). That's a friction point that I have to deal with, and something that I don't think is well acknowledged by the black developers (or at least by black advocates). In all honesty, though, I really just want to get the whole thing over with. It's draining and non-productive for everyone, and I'd like to see the back of it. Hopefully once we're done, we'll end up with black simply being another variation of the annoying code style CI checks that mostly stay out of the way and can be ignored. |
Yea, I hate that too. I've been using https://github.com/keisheiled/flake8-implicit-str-concat locally, so I'll just add that in. The second commit in #10360 does the relevant things. |
Whelp. I did this already -- #10360. If no one says that they want to be reviewing each line, I'd like to just merge that in and get to the other side of this. |
|
I 100000% agree with @pradyunsg personally, but I was doing my best to try and understand other opinions. I ran into this issue on my own diff #9086 - hence why I thought I'd be helping with #10281. Probably it would have been better for me to close the formatting diff and stay out of the way. I didn't realize that there was an intra-maintainer conflict going on. I was genuinely confused by how folks were asking for multiple commits / multiple PRs. It seemed like adding work / CI wasteage with very little additional benefit (which I softly mentioned here #10281 (comment)). I could have stood my ground more, but I'm a newcomer here, so it didn't feel like my place. I love black - because it means that I never need to discuss formatting. Run pre-commit + done. Anyway - I'll steer clear of this formatting discussion from here on so you can resolve. Thanks for being patient with me. |
|
@pradyunsg as far as I'm concerned you can blacken the code base in one go. The sooner the better so we can stop having to resolve merge conflicts. I personally trust black to not change the semantics of the code so I'm not going to read the changes. If there are remaining style issues, we can adjust the code as we meet them during regular maintenance. |
|
I do also think that running black once is the best approach but this need to be agreed/done by cores. It is like removing a bandage, do it quick and it will be less painful than playing with it, gently. |
No description provided.