Blackify some files to show style differences #1186

jfuss · 2019-05-17T15:17:58Z

Issue #, if available:
N/A

Description of changes:
This PR is meant to show what the differences would be if we started to use Black. A good read about benefits and tradeoffs can be found under the Django dep project (dep 0008).

Happy to have a formal write up but wanted to show how this could look in our codebase through a small PR. If the team decides that this is worth while, I can work on formalize this with pre-commit hooks to ensure formatting is always done and therefore we should no longer need to care about formatting :). We could do something similar to how the attrs project is using Black.

Checklist:

Write Design Document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
make pr passes
Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sanathkr · 2019-05-17T16:26:07Z

Orange is the new black!

sriram-mv · 2019-05-20T16:25:12Z

Overall I like it. 👍

sriram-mv · 2019-05-23T15:26:17Z

Shall we adopt this? Given the line length is configurable, I'm okay with us taking this on.

jfuss · 2019-05-23T17:19:17Z

I haven't heard a strict no yet. There is still some work to do here and we need to figure out a good time to do the flip. I am hoping to have this ready sometime early next week.

jfuss · 2019-07-22T05:00:16Z

This should be ready to go.

PR includes:

the current HEAD of development in formatted
updated DEVELOPMENT_GUIDE with instructions on installing black and or pre-commit
updated Makefile with a target to run the formatter
travis running black --check to make sure source code is black compliant
Integrated with pre-commit for those who want to run git hooks with black
Integrated with pyproject.toml to hold our black config. More info in PEP518

sanathkr · 2019-07-25T19:47:44Z

pyproject.toml

+exclude = '''
+
+(
+  /(


Can you also exclude init template directories?

good idea. sure can do.

I have excluded the templates used for sam init. I will un-format them once, I do the update from develop and rerun black dance.

sanathkr · 2019-07-25T19:47:54Z

Makefile

 # Verifications to run before sending a pull request
 pr: init dev
+


remove empty lines

sanathkr · 2019-07-25T19:48:03Z

Makefile

@@ -26,5 +26,10 @@ lint:
 # Command to run everytime you make changes to verify everything works
 dev: flake lint test

+black-format:


call it just black?

sanathkr · 2019-07-25T19:48:18Z

pyproject.toml

+
+[tool.black]
+line-length = 120
+target_version = ['py37', 'py27', 'py36']


remove py27?

I kept it because we still have py27 support :(. So wasn't comfortable making formatting related this and risk breaking it. I just quickly ran it and the only changes were removing u from strings like u"help". For now, I would prefer to just leave it.

sanathkr · 2019-07-25T19:48:35Z

.travis.yml


 install:
  # Install the code requirements
  - make init

 script:
+  # Verify code is black compliant
+  - /tmp/black --check setup.py tests samcli


make black instead?

make black will format the files are write it back to the filesystem. Instead this commands black --check makes sure that the files are formatted and if not the build will fail. This ensures our codebase is always formatted. I can create a make black-check? command or something. I thought about putting it in make pr but didn't want to go through the hassle of pip installing on appveyor too (you can only [pip] install black in 3.6 or above).

sriram-mv · 2019-08-05T03:30:57Z

I wonder if we should run make black on a github post hook or something and completely forget about formatting. it gets auto taken care of.

viksrivat · 2019-08-05T15:53:18Z

If we run it as a github post hook, do we still need to run make lint/flake ?

jfuss · 2019-08-07T04:45:42Z

| I wonder if we should run make black on a github post hook or something and completely forget about formatting. it gets auto taken care of.

@thesriram That is what the pre-commit installation instructions are for. I didn't want to make it required. I don't like pip installing other tools, because it can mess with the global or user installation on a system. So I made this optional but travis does run a check to ensure the files are 'blackified', so we should never be checking in code that is not formatted.

| If we run it as a github post hook, do we still need to run make lint/flake ?

@viksrivat I think we can remove flake8, but would like to keep pylint around until we know black handles everything.

sriram-mv · 2019-08-15T17:15:00Z

DEVELOPMENT_GUIDE.md

+### 2. Install Additional Tooling
+#### Black
+We format our code using [Black](https://github.com/python/black) and verify the source code is black compliant
+in Travis during PRs. You can find installation instructions on [Black's docs](https://black.readthedocs.io/en/stable/installation_and_usage.html).


sriram-mv · 2019-08-15T17:18:40Z

samcli/commands/local/start_lambda/cli.py

+        template,
+        env_vars,
+        debug_port,
+        debug_args,


Not sure why these random empty lines show up here, github formatting?

I don't see any empty lines in the diff. Must be github?

sanathkr · 2019-09-04T15:44:41Z

Makefile


 # Verifications to run before sending a pull request
-pr: init dev
+pr: init dev


Why isn't black run as part of make pr?

the make black command requires you to commit the changes. So I didn't want the make pr to run that and make it look like it works. I can add another make command that does the check instead, which is probably what we want anyways.

If you install the git hooks, running the check on pr is just redundant. This is because you can't push unless black passes.

sanathkr

LGTM as long as CI passes ⬛️🏴

jfuss requested review from sanathkr, sriram-mv and awood45 May 17, 2019 15:18

jfuss force-pushed the adopt-black branch 5 times, most recently from 5657b67 to 11b42e8 Compare July 22, 2019 04:17

jfuss marked this pull request as ready for review July 22, 2019 05:00

sanathkr suggested changes Jul 25, 2019

View reviewed changes

jfuss force-pushed the adopt-black branch from d2fb92a to c84a3be Compare August 15, 2019 16:52

sriram-mv approved these changes Aug 15, 2019

View reviewed changes

jfuss force-pushed the adopt-black branch from c84a3be to d9614d7 Compare August 28, 2019 19:24

jfuss added 7 commits September 4, 2019 08:19

Adding file for to make black supported

b5935de

remove flake8

ff65d95

Update appveyor linux to run black --check

e2e53a1

Run black on source

3f70dca

travis -> Appveyor

27c276e

fix pylint errors

547874b

Rebase/blackify new commits into develop

6c4ed24

jfuss force-pushed the adopt-black branch from 2e253a7 to 6c4ed24 Compare September 4, 2019 15:28

sanathkr reviewed Sep 4, 2019

View reviewed changes

sanathkr approved these changes Sep 4, 2019

View reviewed changes

sanathkr merged commit a83aa9e into aws:develop Sep 4, 2019

jfuss deleted the adopt-black branch September 4, 2019 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blackify some files to show style differences #1186

Blackify some files to show style differences #1186

jfuss commented May 17, 2019

sanathkr commented May 17, 2019

sriram-mv commented May 20, 2019

sriram-mv commented May 23, 2019

jfuss commented May 23, 2019

jfuss commented Jul 22, 2019

sanathkr Jul 25, 2019

jfuss Jul 25, 2019

jfuss Jul 25, 2019

sanathkr Jul 25, 2019

jfuss Jul 25, 2019

sanathkr Jul 25, 2019

jfuss Jul 25, 2019

sanathkr Jul 25, 2019

jfuss Jul 25, 2019

sanathkr Jul 25, 2019

jfuss Jul 25, 2019

sriram-mv commented Aug 5, 2019

viksrivat commented Aug 5, 2019

jfuss commented Aug 7, 2019

sriram-mv Aug 15, 2019

jfuss Aug 28, 2019

sriram-mv Aug 15, 2019

jfuss Aug 28, 2019

sanathkr Sep 4, 2019

jfuss Sep 4, 2019

jfuss Sep 4, 2019

sanathkr left a comment

		# Verifications to run before sending a pull request
		pr: init dev

Blackify some files to show style differences #1186

Blackify some files to show style differences #1186

Conversation

jfuss commented May 17, 2019

sanathkr commented May 17, 2019

sriram-mv commented May 20, 2019

sriram-mv commented May 23, 2019

jfuss commented May 23, 2019

jfuss commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sriram-mv commented Aug 5, 2019

viksrivat commented Aug 5, 2019

jfuss commented Aug 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanathkr left a comment

Choose a reason for hiding this comment