Blacken

[ocefpaf]

@Conengmo let's merge #51 before this one to avoid disruptions in that PR.

Note that this seems quite intrusive but there are virtually no functional change. If you approve this here we can probably do the same in folium. The arguments in favor os this PR are:

• blacken the code to never deal with code style ever again;
• outsource the linting to pytest and use pytest-xdist for quicker test times;
• drop legacy Python and support Python >=3.6 only.

We can later add some instructions on how to use the makefile which will make reviews and maintenance much easier.

[Conengmo]

Hi Filipe, to be honest I'm a bit hesitant about this. I feel like Black is a bit heavy handed. I've used autopep8 before with some options, that was lighter to my taste. But autoformatting in general seems like a good idea!

• I thought we used single quotes in our code
• Maybe set the maximum line length to 100 for the autoformatter?

I like using Stickler because it makes Github comments. If we do linting on Travis we need to dive into the Travis report to see the issues.

Hmm I now see we don't have Stickler here on Branca... Then it seems fine to switch to Pytest if you say that's better.

I have no experience with pytest-xdist, can we use that on the free Travis service?

Dropping Python 2 support needs a bit more care I think given the large amount of our usebase still on Py2. I'm thinking we drop it first on folium, and then on branca. We don't make that much changes to branca so I don't think there's much reason to drop it.

[ocefpaf]

Hi Filipe, to be honest I'm a bit hesitant about this. I feel like Black is a bit heavy handed. I've used autopep8 before with some options, that was lighter to my taste.

I used to think the same until I started using it as a commit git-hook. I really like it now.

But autoformatting in general seems like a good idea!

Sure we can use another approach.

• I thought we used single quotes in our code

Yeah. Enforcing " is the part that I don't like about black but come to accept. I'll check if they added an option to change that or not, but I don't think so based on the premise of the package.

• Maybe set the maximum line length to 100 for the autoformatter?

Sure.

I like using Stickler because it makes Github comments. If we do linting on Travis we need to dive into the Travis report to see the issues.

The goal is actually to have a Makefile that people can check before committing, or teaching them to create the git-hook. Then stickler and the test will be obsolete.

I have no experience with pytest-xdist, can we use that on the free Travis service?

Travis-CI does not promise 2 cores but usually have more than that available. In my experience that is OK and the tests do tun faster.

Dropping Python 2 support needs a bit more care I think given the large amount of our usebase still on Py2. I'm thinking we drop it first on folium, and then on branca. We don't make that much changes to branca so I don't think there's much reason to drop it.

No problem. My thinking was that dropping here, where the stakes are low, would be a nice exercise before dropping on folium.

[Conengmo]

I really like it now.

I'll try it out! Let me get back to you.

The goal is actually to have a Makefile that people can check before committing, or teaching them to create the git-hook. Then stickler and the test will be obsolete.

Sounds good! I don't know yet how the work flow will work. If someone makes a PR without running the autoformatter, is a commit added with the autoformatter changes or something?

My thinking was that dropping here, where the stakes are low, would be a nice exercise before dropping on folium.

branca is a dependency of folium, so if we drop it here it also affects folium users. We need to make sure Python 2 users can only install the last Py2 compatible version.

[ocefpaf]

I'll try it out! Let me get back to you.

No pressure but I've come to appreciate how it just removes the worry about styling and linting.

Sounds good! I don't know yet how the work flow will work. If someone makes a PR without running the autoformatter, is a commit added with the autoformatter changes or something?

We can do that but the so far what I'm using this Makefile and telling people to run make check before submitting a PR.

branca is a dependency of folium, so if we drop it here it also affects folium users. We need to make sure Python 2 users can only install the last Py2 compatible version.

Indeed but the release cycle is so slow that I don't see another branca release, unless we unlock https://github.com/python-visualization/branca/pull/51, before we drop it even in folium.

[Conengmo]

I tried black and I must say I think it's a good idea.

Some remarks:

• I like adding the -t argument so it's explicit which versions we support: black branca -t py35 -t py36 -t py37
• The current version has a bug with -t py27: it adds whitespace between print and (. It was fixed two days ago :) but not yet in a release. Guess we shouldn't run it for py27 then.
• It doesn't format long strings unfortunately. I tried YAPF but they don't seem to have that functionality either. We'll have to do that manually.
• I feel like the line length of 80 is too restrictive, especially since we work with all these long string templates. Maybe blacks default of 88 is a good compromise?

I added the flake8 config to setup.cfg, so it's a bit more clear where that's coming from:

[pytest]
flake8-max-line-length = 88
flake8-max-complexity = 14
flake8-ignore = E203 W503
# E203 - whitespace before ':'
# W503 - line break before binary operator

If we are to drop Python 2.7 for the next release, we should include some safety checks in setup.py and __init__.py. I worked on that for folium here: https://github.com/python-visualization/folium/pull/1100

With this PR it's a bit hard to see what's going on. Is it too much trouble for you to split it? I'm thinking we should tackle the dropping Py2 first and separately.

[ocefpaf]

I tried black and I must say I think it's a good idea.

Glad you liked it.

• I like adding the -t argument so it's explicit which versions we support: black branca -t py35 -t py36 -t py37

Sure. But I wonder if there are many differences for py36/37 though.

• The current version has a bug with -t py27: it adds whitespace between print and (. It was fixed two days ago :) but not yet in a release. Guess we shouldn't run it for py27 then.

Indeed but this PR also proposes to drop py27 :smiley_cat:

• It doesn't format long strings unfortunately. I tried YAPF but they don't seem to have that functionality either. We'll have to do that manually.

I missed that. Thanks for pointing it out!

• I feel like the line length of 80 is too restrictive, especially since we work with all these long string templates. Maybe blacks default of 88 is a good compromise?

Sure.

I added the flake8 config to setup.cfg, so it's a bit more clear where that's coming from:

[pytest]
flake8-max-line-length = 88
flake8-max-complexity = 14
flake8-ignore = E203 W503
# E203 - whitespace before ':'
# W503 - line break before binary operator

:+1:

If we are to drop Python 2.7 for the next release, we should include some safety checks in setup.py and __init__.py. I worked on that for folium here: python-visualization/folium#1100

I can port those changes here. Thanks!

With this PR it's a bit hard to see what's going on. Is it too much trouble for you to split it?

Not at all. I'll do that soon.

I'm thinking we should tackle the dropping Py2 first and separately.

Makes perfect sense. (I never really thought we about merging this as is. It was more of a conversation starter for some the code changes here.)

BTW, if we are going to do something like this in folium, we should merge as many PRs as we can before it to avoid merge conflicts b/c this touches the whole code base.

[Conengmo]

Hi @ocefpaf I was wondering, what's the benefit of using pytest to run flake8 over running flake8 directly?

[ocefpaf]

Hi @ocefpaf I was wondering, what's the benefit of using pytest to run flake8 over running flake8 directly?

It caches the results and run only on the changed files. Good for running multiple times when you are editing the code. Also, it becomes part of the actual tests with better errors and messages.

PS: I'll work on this PR soon. (I'm travelling this week and I'll refactor and re-send it as soon as I get back.)

[Conengmo]

Thanks for explaining! That does sound useful.

[ocefpaf]

@Conengmo with #58 merged can we revisit this one?

[ocefpaf]

I'll resubmit this later.