Blackify code base #1799

Cnidarias · 2018-10-08T16:07:49Z

Letting black run over everything

Add documentation.
Add a changelog entry.
Add your name in the contributors file.

So in light of my other pull request I suggested that we run black over everything and @leplatrem seemed to be in favor so here we are.

I figured it would be best if we did this before adding all the type annotations to not have too many changes at once.

I let black run over everything, adding a pre-commit (https://pre-commit.com) config file as well as a config file for black.

I also had to add an ignore E203 in order for flake8 to be happy.

If this is okay then I would still have to add some documentation on how to set things up for development but I do think the result is actually quite nice!

Thoughts?

leplatrem · 2018-10-08T19:39:48Z

@Natim @glasserc ?

leplatrem · 2018-10-08T19:52:16Z

Note: @peterbe recommends therapist https://github.com/peterbe/hashin/pull/80/files

leplatrem · 2018-10-08T20:08:17Z

Be careful commit edbf3b8 should not be part of this PR

Cnidarias · 2018-10-11T06:01:50Z

So I basically just added the stuff you linked -- not sure I did it correctly though especially regarding therapist/travis/tox

But it seems like its fine?

Still need to add some documentation though.

leplatrem · 2018-10-11T13:26:51Z

Excellent! Let's get the documentation part updated for developers in the community section and then we can iterate :) We should probably add the necessary deps in the dev-requirements.txt right?

The diff is hard to review 😅

Cnidarias · 2018-10-11T16:58:08Z

Hey,

yes the diff is quite massive.
This does beg another question, when looking at the diffs alot of changes are changing single quotes to double quotes

Black does provide the option to omit normalizing quotes.

After testing it - the diff would still be massive. When omitting the quite normalization black still touches all but 24 files.

I have also added some documentation now.
I would prefer to rebase once we are happy with the documentation and everything else because there will always be merge conflicts as of now.

Edit:
When adding black to the dev-requirements.txt

Travis will fail the py35 build as black requires python 3.6

leplatrem · 2018-10-15T10:00:41Z

docs/community.rst

+6. When you're done making changes, format your code with `black <https://github.com/ambv/black>`_ (black requires python >= 3.6)::
+
+    python -m pip install black
+    black .


I would suggest to install black only and run it via tox -e lint (tox and therapist will be installed via dev-requirements)

leplatrem · 2018-10-15T10:01:00Z

docs/community.rst

-7. Commit your changes and push your branch to GitHub::
+8. (Optional) Install a git hook with `therapist <https://github.com/rehandalal/therapist>`_ ::
+
+    python -m pip install therapist


this line can be omitted since already in dev-requirements

leplatrem · 2018-10-15T10:03:36Z

Almost there! 🎉

I would like to avoid configuration of black if possible. Double quotes are fine.
Since python 3.5 is still officially supported, we have to adapt. Suggesting its installation manually in the dev docs is fine IMO

Thanks 🙏

leplatrem · 2018-10-17T09:19:28Z

Let me know if you need help or support ;)

Actually I would be very interested with this PR, because I started to work on #710 using xargs sed like an animal :)

Cnidarias · 2018-10-17T10:20:28Z

Hey,

sorry about not getting back to you on this earlier.

We should probably move therapist and black to its own requirements.txt file (maybe something like lint-requirements.txt) as, like I said travis will fail the py3.5 build because it cannot install black.

I will adjust the documentation accordingly if that is okay.

I will only be able to work at this later tonight though after work

… merge

Cnidarias · 2018-10-18T05:36:36Z

Okay - so I went ahead and got rid of my changes because I did not feel like merging 40+ merge conflicts.

This commit now has everything we had earlier with the additional changes that we now have a lint-requirements.txt file as well as adapting the documentation.

The code has not yet been formatted with black so it will be easier for me to rebase it once we are ready to merge.

leplatrem

Don't you think that we could just get rid of flake8 on py35? I mean, if we do lint check on python 3.6 it's good enough no?

leplatrem · 2018-10-18T07:47:36Z

lint-requirements.txt

@@ -0,0 +1,2 @@
+therapist==1.5.0
+black==18.9b0


Maybe we could add:

flake8 setuptools<36

BTW @Natim do you remember why setuptools<36?

leplatrem · 2018-10-18T07:47:50Z

docs/community.rst

+8. (Optional) Install a git hook::
+
+    therapist install
+


looks like an extra line

Don't you have to tell therapist to create the hook in your .git dir?

blank line sorry :)

leplatrem · 2018-10-18T07:48:42Z

tox.ini

@@ -16,11 +16,12 @@ deps =
    statsd
 install_command = pip install {opts} {packages}

-[testenv:flake8]
-commands = flake8 kinto tests docs/conf.py


if it's mentioned in travis, we should keep it flake8 (py35).
But I would in favor of removing it in travis too instead :)

Yes I should have removed it in travis.

leplatrem

Great!! Thank you for the hard work :) You can format and I'll press the merge button :)

Cnidarias · 2018-10-18T11:27:45Z

Alright - I ran black and merged the conflict

leplatrem · 2018-10-18T12:12:10Z

:) here we go :) thanks again ;)

Cnidarias force-pushed the black branch from dd7f033 to b2438de Compare October 10, 2018 10:47

leplatrem reviewed Oct 15, 2018

View reviewed changes

Revert blackifying code so that I only run black when we are ready to…

8e5a3a7

… merge

Cnidarias force-pushed the black branch from 2568385 to 8e5a3a7 Compare October 18, 2018 05:34

leplatrem reviewed Oct 18, 2018

View reviewed changes

Move deps around remove flake8 from travis/tox

96a0192

leplatrem approved these changes Oct 18, 2018

View reviewed changes

Cnidarias and others added 2 commits October 18, 2018 13:24

Reformated code with black, removed blank line from documentation

ec8fbbf

Merge branch 'master' into black

900d787

Added ignores to flake8 so that black and flake8 can be happy

2fb5d0a

leplatrem merged commit efc027f into Kinto:master Oct 18, 2018

Cnidarias deleted the black branch October 18, 2018 12:59

Natim mentioned this pull request Jan 13, 2020

Add isort support. spiral-project/ihatemoney#527

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blackify code base #1799

Blackify code base #1799

Cnidarias commented Oct 8, 2018

leplatrem commented Oct 8, 2018 •

edited

Loading

leplatrem commented Oct 8, 2018

leplatrem commented Oct 8, 2018

Cnidarias commented Oct 11, 2018

leplatrem commented Oct 11, 2018

Cnidarias commented Oct 11, 2018 •

edited

Loading

leplatrem Oct 15, 2018

leplatrem Oct 15, 2018

leplatrem commented Oct 15, 2018

leplatrem commented Oct 17, 2018

Cnidarias commented Oct 17, 2018

Cnidarias commented Oct 18, 2018

leplatrem left a comment

leplatrem Oct 18, 2018

leplatrem Oct 18, 2018

Cnidarias Oct 18, 2018

leplatrem Oct 18, 2018

leplatrem Oct 18, 2018 •

edited

Loading

Cnidarias Oct 18, 2018

leplatrem left a comment

Cnidarias commented Oct 18, 2018

leplatrem commented Oct 18, 2018

		@@ -0,0 +1,2 @@
		therapist==1.5.0
		black==18.9b0

Blackify code base #1799

Blackify code base #1799

Conversation

Cnidarias commented Oct 8, 2018

leplatrem commented Oct 8, 2018 • edited Loading

leplatrem commented Oct 8, 2018

leplatrem commented Oct 8, 2018

Cnidarias commented Oct 11, 2018

leplatrem commented Oct 11, 2018

Cnidarias commented Oct 11, 2018 • edited Loading

leplatrem Oct 15, 2018

Choose a reason for hiding this comment

leplatrem Oct 15, 2018

Choose a reason for hiding this comment

leplatrem commented Oct 15, 2018

leplatrem commented Oct 17, 2018

Cnidarias commented Oct 17, 2018

Cnidarias commented Oct 18, 2018

leplatrem left a comment

Choose a reason for hiding this comment

leplatrem Oct 18, 2018

Choose a reason for hiding this comment

leplatrem Oct 18, 2018

Choose a reason for hiding this comment

Cnidarias Oct 18, 2018

Choose a reason for hiding this comment

leplatrem Oct 18, 2018

Choose a reason for hiding this comment

leplatrem Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Cnidarias Oct 18, 2018

Choose a reason for hiding this comment

leplatrem left a comment

Choose a reason for hiding this comment

Cnidarias commented Oct 18, 2018

leplatrem commented Oct 18, 2018

leplatrem commented Oct 8, 2018 •

edited

Loading

Cnidarias commented Oct 11, 2018 •

edited

Loading

leplatrem Oct 18, 2018 •

edited

Loading