style: adopt black #1876

CaselIT · 2021-03-03T22:54:14Z

Just to get the ball rolling.

Tried to run black ~~20.8b1~~ 21.5b1 with the configuration

[tool.black]
    target-version = ["py35"]
    skip-string-normalization = true
    line-length = 88

The skip-string-normalization avoids the conversion ' to " if we want to keep the current flake8 rules

TODO:

re-run with line length 88
cleanup string concatenation in the same line (like 'foo bar ' 'baz foobar')
review where we need to add / remove tailing commas to improve the styling
update the documentation with the changes done to the example folder

Closes #1453

codecov · 2021-03-03T22:55:23Z

Codecov Report

Merging #1876 (bdddbb4) into master (c550eb3) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1876   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        63           
  Lines         6610      6610           
  Branches      1067      1067           
=========================================
  Hits          6610      6610

Impacted Files	Coverage Δ
falcon/__init__.py	`100.00% <ø> (ø)`
falcon/asgi/structures.py	`100.00% <ø> (ø)`
falcon/inspect.py	`100.00% <ø> (ø)`
falcon/media/validators/jsonschema.py	`100.00% <ø> (ø)`
falcon/responders.py	`100.00% <ø> (ø)`
falcon/response_helpers.py	`100.00% <ø> (ø)`
falcon/testing/__init__.py	`100.00% <ø> (ø)`
falcon/testing/test_case.py	`100.00% <ø> (ø)`
falcon/util/deprecation.py	`100.00% <ø> (ø)`
falcon/util/structures.py	`100.00% <ø> (ø)`
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c550eb3...bdddbb4. Read the comment docs.

examples/look/look/images.py

examples/things_advanced.py

kgriffs · 2021-03-09T16:44:36Z

I am going to delay this for post 3.0 but I do think we should move in this direction. Let's just make sure we have as part of the process a review to tweak the occasional oddity.

CaselIT · 2021-03-09T17:14:09Z

I am going to delay this for post 3.0 but I do think we should move in this direction. Let's just make sure we have as part of the process a review to tweak the occasional oddity.

yes sure, I was not targeting this release, it was just to start looking into it

CaselIT · 2021-05-13T19:37:16Z

I've updated the change with the latest black.

Regarding the line length it can easily customized. I personally usually use 100, but also black default of 88 is fine. I find 79-80 a bit on the narrow side.

kgriffs · 2021-05-13T21:22:05Z

The default of 88 is fine, and avoids excessive line wrapping esp. for deeply nested control structures. If we increased the limit much further I would argue for a separate standard for keeping comments shorter (even if we have to write our own checker for it).

Regardless, I'll take a look through the diff soon and see how things look now.

CaselIT · 2021-05-13T22:28:18Z

I'll re-reun with the 88 line.

If you are not familiar with black, note that you can tell it to split things in multiple lines by setting a comma at the end.
Like

def foo(x,):
  pass
# becomes
def foo(
  x,
):
  pass

So we may need to add / remove some to get the desired formatting.

https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html?highlight=comma#the-magic-trailing-comma

vytas7 · 2021-05-14T11:19:40Z

I'm a bit orthodox when it comes to line length, 100+ sounds a bit too long.

But I'm fine with black's 88, it does sound like a good compromise, and... if we're going to use black, we don't need to care about manually keeping lines at 79 or 88 anyway, that's what black is for.

CaselIT · 2021-05-14T12:24:17Z

Ok, I initially went with 100 both because we do have some lines that are on the long side in the source and it's what I usually use.

I'll re-run with the default 88 and update this.

CaselIT · 2021-05-14T19:08:54Z

Updated with 88. From the change lines it seems that our average line was a bit longer, since this version adds a 1000 new lines.

For reference the previous with length 100 was more or less a wash regarding line count

kgriffs

Comments inline. I am still favorable to adopting black modulo a few tweaks. Also, note that f we miss any strange formatting issues before merging this, we can always fix those up over time, as we discover them in the course of maintaining the project.

examples/look/look/images.py

examples/look/tests/test_app.py

falcon/app.py

falcon/media/validators/jsonschema.py

CaselIT · 2021-05-25T21:24:26Z

I'll wait for #1709 before rebasing and polishing this

docs/user/quickstart.rst

CaselIT · 2021-05-28T22:04:31Z

@kgriffs @vytas7 I've updated the docs to use the new formatting, bug I'm not sure I updated all of them.

I've also made a quick pass to the files to check that no funny formatting was added. For this I think we can correct eventual leftover in the future

vytas7 · 2021-05-30T17:44:23Z

setup.cfg

@@ -85,7 +85,7 @@ filterwarnings =
 [flake8]
 max-complexity = 15
 exclude = .ecosystem,.eggs,.git,.tox,.venv,build,dist,docs,examples,falcon/bench/nuts
-ignore = F403,W504
+extend-ignore = F403,W504,E203,I202
 max-line-length = 99


Shouldn't we now decrease this to 88?

Yes, I guess so

Note that we have quite a bit of comment/documentation that are over 88 chars

tests/asgi/test_boundedstream_asgi.py

kgriffs

💥

vytas7

This is getting almost ready 👍

I just had a handful of open questions (commented inline).

And a more general question: should we try updating Cython code for the delimited stream reader etc? When I wrote it, it was loosely based on the Python version, with changes specific to Cython. Right now, that parity would be disturbed a bit. OTOH mangling that code now after the fact is just a (an unnecessary?) risk to introduce regressions.

falcon/routing/compiled.py

falcon/util/uri.py

falcon/vendor/mimeparse/mimeparse.py

CaselIT · 2021-06-14T19:48:25Z

And a more general question: should we try updating Cython code for the delimited stream reader etc? When I wrote it, it was loosely based on the Python version, with changes specific to Cython.

black does not support cython, so the formatting would be manual.

Personally I think we could leave it as is or update as needed as a follow on change (maybe in the same issue as the one setting flake line length to 88)

kgriffs

👍

vytas7

Here we go then...

CaselIT · 2021-06-23T09:00:22Z

I've opened #1926 to do the follow up on this change.

Please add there if there were other deferred points from this change. I'll try to get to it this we

CaselIT commented Mar 3, 2021

View reviewed changes

examples/look/look/images.py Outdated Show resolved Hide resolved

vytas7 reviewed Mar 4, 2021

View reviewed changes

examples/things_advanced.py Outdated Show resolved Hide resolved

kgriffs added the pr::delay-for-next-release label Mar 9, 2021

kgriffs removed the pr::delay-for-next-release label May 10, 2021

CaselIT force-pushed the try_black branch 2 times, most recently from a742c49 to 9d1de91 Compare May 13, 2021 19:33

CaselIT requested review from vytas7 and kgriffs May 13, 2021 19:36

CaselIT force-pushed the try_black branch from 9d1de91 to 14d2a39 Compare May 14, 2021 19:04

kgriffs requested changes May 24, 2021

View reviewed changes

CaselIT added 4 commits May 28, 2021 22:28

style: run black

4009629

style: fix multiline strings

5e2f95e

style: improve readability of examples

c421cf2

chore: update tox, setup and clear flake8 errors

9442c14

CaselIT force-pushed the try_black branch from 14d2a39 to 9442c14 Compare May 28, 2021 20:58

docs: update quickstart docs

434ad76

CaselIT requested a review from kgriffs May 28, 2021 22:01

CaselIT commented May 28, 2021

View reviewed changes

docs/user/quickstart.rst Show resolved Hide resolved

chore: add black gate to tests

67ea731

CaselIT force-pushed the try_black branch from 205a43f to 67ea731 Compare May 28, 2021 22:06

style: re-run black

c93e443

vytas7 reviewed May 30, 2021

View reviewed changes

tests/asgi/test_boundedstream_asgi.py Show resolved Hide resolved

kgriffs previously approved these changes Jun 8, 2021

View reviewed changes

vytas7 reviewed Jun 14, 2021

View reviewed changes

falcon/routing/compiled.py Outdated Show resolved Hide resolved

falcon/util/uri.py Show resolved Hide resolved

falcon/vendor/mimeparse/mimeparse.py Outdated Show resolved Hide resolved

style: improve formatting, exclude vendored dependencies

bc553d9

CaselIT dismissed kgriffs’s stale review via bc553d9 June 14, 2021 19:58

CaselIT requested review from vytas7 and kgriffs June 15, 2021 19:16

test: fix missing dependencies for mypy

bdddbb4

CaselIT force-pushed the try_black branch from ca782b1 to bdddbb4 Compare June 15, 2021 19:18

kgriffs approved these changes Jun 22, 2021

View reviewed changes

vytas7 changed the title ~~style: try running black~~ style: adopt black Jun 23, 2021

vytas7 approved these changes Jun 23, 2021

View reviewed changes

vytas7 merged commit 7efb46f into falconry:master Jun 23, 2021

CaselIT deleted the try_black branch June 23, 2021 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: adopt black #1876

style: adopt black #1876

CaselIT commented Mar 3, 2021 •

edited

Loading

codecov bot commented Mar 3, 2021 •

edited

Loading

kgriffs commented Mar 9, 2021

CaselIT commented Mar 9, 2021

CaselIT commented May 13, 2021 •

edited

Loading

kgriffs commented May 13, 2021

CaselIT commented May 13, 2021

vytas7 commented May 14, 2021

CaselIT commented May 14, 2021

CaselIT commented May 14, 2021

kgriffs left a comment

CaselIT commented May 25, 2021

CaselIT commented May 28, 2021

vytas7 May 30, 2021

CaselIT May 30, 2021

CaselIT Jun 2, 2021

kgriffs left a comment

vytas7 left a comment

CaselIT commented Jun 14, 2021

kgriffs left a comment

vytas7 left a comment

CaselIT commented Jun 23, 2021

style: adopt black #1876

style: adopt black #1876

Conversation

CaselIT commented Mar 3, 2021 • edited Loading

codecov bot commented Mar 3, 2021 • edited Loading

Codecov Report

kgriffs commented Mar 9, 2021

CaselIT commented Mar 9, 2021

CaselIT commented May 13, 2021 • edited Loading

kgriffs commented May 13, 2021

CaselIT commented May 13, 2021

vytas7 commented May 14, 2021

CaselIT commented May 14, 2021

CaselIT commented May 14, 2021

kgriffs left a comment

Choose a reason for hiding this comment

CaselIT commented May 25, 2021

CaselIT commented May 28, 2021

vytas7 May 30, 2021

Choose a reason for hiding this comment

CaselIT May 30, 2021

Choose a reason for hiding this comment

CaselIT Jun 2, 2021

Choose a reason for hiding this comment

kgriffs left a comment

Choose a reason for hiding this comment

vytas7 left a comment

Choose a reason for hiding this comment

CaselIT commented Jun 14, 2021

kgriffs left a comment

Choose a reason for hiding this comment

vytas7 left a comment

Choose a reason for hiding this comment

CaselIT commented Jun 23, 2021

CaselIT commented Mar 3, 2021 •

edited

Loading

codecov bot commented Mar 3, 2021 •

edited

Loading

CaselIT commented May 13, 2021 •

edited

Loading