Refs #1184 -- Reformatted code with Black. #1224
Conversation
|
In a subsequent PR, I'll add black in the pre-commit hooks and also ensure that this commit can be ignored in git-blame. |
|
Have read to comments form @carltongibson on this issue ? Please in any case wait for reviews before merge this PR. There's a lot of changed file so it can be take long time to review. All contribute in review this PR are welcome. |
Yes, I did :) The commit on this PR is from dj-bot as Carlton mentioned. I'll follow up with a PR as mentioned to mark this commit to be ignored in git-blame rev. Let me know if there is anything else.
Of course :)
|
There was a problem hiding this comment.
Hey @CuriousLearner. Thanks for this.
The actual "Applied Black" commit seems fine — ...
Can you look at https://github.com/django/django/pull/15387/commits — we need to adjust the isort and flake8 configs, and add a Black config, and ensure that it's applied going forward (otherwise we drift away). And so on...
Can you add (a) separate commit(s) for those so we can take them as a batch?
🎁
|
Sure, I'll have a look later in the evening and get this done. Thanks! |
87999be to
6e6d918
Compare
d3dcc63 to
3332acb
Compare
ba901d6 to
09188e4
Compare
|
Hi @carltongibson 👋🏼 ! I tried to incorporate your review. The default line-length is now standardized to 88 chars. I had to do some manual changes in the actual The actual black commit(s) lint action failed since the config was later adjusted in the subsequent commit, but please let me know if I should adjust the config in format black commit too to make everything passed (I'm not sure what is the preference here). Many thanks! cc @pauloxnet |
|
@CuriousLearner thanks for all your work here. The PR seems OK to me but I prefer to wait @carltongibson or @felixxm review here because they already managed the similar PR in the Django code and also because its a huge PR. |
|
@CuriousLearner can you rebase this PR and read the comments above ? |
|
@CuriousLearner Sorry for the slow follow-up. If you can rebase and address the comments I will take a proper look next week. Super effort 🎁 |
There was a problem hiding this comment.
OK, I think this looks good. @CuriousLearner — have you got the bandwidth to update? (If not I can do it!) I think we should get it in then.
Ref the pre-commit config, @adamchainz had me use the bot/GitHub app on the Daphne repo, and that works quite well… — I was going to extend it to the other Channels repos, if that's a measure. If @django/djangoproject-com-maintainters think so, I can add maybe it here too? (Permissions are per-repo…)
|
Hey @carltongibson Sorry for the late reply. I had to wrap-up a project this week at work. I'll get this done tomorrow morning. Cheers! |
800b40c to
532da37
Compare
|
I've rebased the PR and address @pauloxnet 's review. Please have a look now. |
I agree, please do it @carltongibson 🙏🏻 |
Hi @CuriousLearner and thanks for your hard work on this PR. I've just reviewed again and approved, but let wait for @carltongibson review and edits. |
|
After setting up pre-commit.ci, we can drop the tox jobs to run black/flake8/isort, and related github actions steps. |
OK, I've just added the pre-commit app to this repo. (The was a 400 error in the process, but logging in there, it says it worked... 😬) |
|
@CuriousLearner Just FYI, since I saw your reaction, I'm rebasing this (and will experiment with a "bad commit") now. |
22d41c9 to
acc32b2
Compare
These are now run via the pre-commit integration.
ce6c725 to
1b0e1a3
Compare
There was a problem hiding this comment.
This looks good to me. Thanks for your work @CuriousLearner 🏅
I've rebased and adjusted slightly:
- Added a line-length ignore for the docs test file with the long fixtures.
- Dropped the GHA linter step as per Adam's comment. I kept the tox jobs, since they're handy for me at least to run locally. (I know, don't @-me 😜)
For me this is ready to be (rebase and) merged as three commits. @pauloxnet given that I edited can I just ask you to glance and do that? Thanks!
| per-file-ignores = | ||
| docs/tests.py: E501 |
There was a problem hiding this comment.
This is for the docs fixtures in this file, which have a longer length but don't seem worth manually reformatting.
See https://results.pre-commit.ci/run/github/690903/1669801921.cpckKRxoTl-Z01SxoOmUxQ
@carltongibson as you asked, after reviewing I rebased and merged using the Github action for that. I hope that's exactly what you asked me to do. :-) Thanks again to @CuriousLearner for his works here. |
|
@pauloxnet perfect thanks! @CuriousLearner if you wanted to follow-up adding the Black commit to a |
|
Thanks for the review and getting this in @pauloxnet & @carltongibson @carltongibson I've added the subsequent PR here: #1280 Thanks 🙌🏼 |
This formats the code with black and also ensures that flake8 doesn't conflict with its changes.