Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt black code style #485

Merged
merged 8 commits into from
Oct 20, 2020
Merged

Adopt black code style #485

merged 8 commits into from
Oct 20, 2020

Conversation

pwildenhain
Copy link
Member

@pwildenhain pwildenhain commented Oct 13, 2020

Related to Closes #481

While there are a ton of diffs, I'm noticing some patterns:

  1. Many changes related to preferring double quotes over single quotes
  2. Many changes related to indenting long lines
  3. Many diffs in the dialects package alone

There are also a few linting CI failures, the flake8-black plugin mentions that there are a few flake8 rules you would want to disable when using alongside black

What do you think @alanmcruickshank? Do you think black is an improvement?
If so, do you think we should go for it now, or wait?
If not, do you have other suggestions for code formatters? I feel like a code linting/formatting library should have something like this 😁 😉

@pwildenhain pwildenhain added the question Further information is requested label Oct 13, 2020
@pwildenhain pwildenhain self-assigned this Oct 13, 2020
@alanmcruickshank
Copy link
Member

HA! Yeah.... I take your point on lots of diffs!

I've skimmed through the suggested changes, and on the whole it looks pretty reasonable.

I feel like a code linting/formatting library should have something like this 😁 😉

...well exactly.

Given the scale of this we should plan when to merge, but it's going to be painful whenever we do it, so we're just going to have to lean into that.

There are also a few linting CI failures, the flake8-black plugin mentions that there are a few flake8 rules you would want to disable when using alongside black

What are the conflicts? I think they're the things we should hash out now and then the rest is just a case of timing.

... oh and we should totally make sure we've got a markdown badge for this if we're going to the effort of doing it... 🤷

[![Code style: black](https://img.shields.io/badge/code%20style-black-000000.svg)](https://github.com/psf/black)

@pwildenhain
Copy link
Member Author

What are the conflicts?

This is the recommended configuration settings for using flake8-black

[flake8]
# Recommend matching the black line length (default 88),
# rather than using the flake8 default of 79:
max-line-length = 88
extend-ignore =
    # See https://github.com/PyCQA/pycodestyle/issues/373
    E203,

E203 is the reason the linting CI is failing now (spaces before semi-colons)

And you know I want the fancy badge 📛 ✨ 😎

we should plan when to merge, but it's going to be painful whenever we do it

If you want, I can quickly add the plugin, with configuration, and the new badge. Or we can close this for now, and come back to this later.

For what it's worth, black only took 2 seconds to run, which was awesome ⚡

@alanmcruickshank
Copy link
Member

Let's do it. You only live once 🤘

@pwildenhain pwildenhain added enhancement New feature or request and removed question Further information is requested labels Oct 14, 2020
@pwildenhain pwildenhain marked this pull request as ready for review October 14, 2020 16:35
@pwildenhain pwildenhain changed the title Try out black Adopt black code style Oct 14, 2020
@pwildenhain pwildenhain requested a review from a team October 14, 2020 16:47
@pwildenhain
Copy link
Member Author

We're good to go! Pinging 🔔 the other maintainers so they're aware of this change

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #485 into master will not change coverage.
The diff coverage is 92.21%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #485   +/-   ##
=======================================
  Coverage   92.91%   92.91%           
=======================================
  Files          29       29           
  Lines        4786     4786           
=======================================
  Hits         4447     4447           
  Misses        339      339           
Flag Coverage Δ
#py36 92.80% <92.21%> (ø)
#py37 92.80% <92.21%> (ø)
#py38 92.74% <92.21%> (ø)
#py39 92.74% <92.21%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/sqlfluff/parser/segment_generator.py 100.00% <ø> (ø)
src/sqlfluff/rules/config_info.py 100.00% <ø> (ø)
src/sqlfluff/rules/std.py 94.25% <ø> (ø)
src/sqlfluff/dialects/base.py 87.09% <42.85%> (ø)
src/sqlfluff/parser/segments_base.py 91.92% <80.00%> (ø)
src/sqlfluff/parser/segments_common.py 91.55% <80.00%> (ø)
src/sqlfluff/cli/formatters.py 95.83% <86.20%> (ø)
src/sqlfluff/parser/grammar.py 89.60% <87.83%> (ø)
src/sqlfluff/parser/match.py 87.69% <88.88%> (ø)
src/sqlfluff/cli/commands.py 90.83% <91.01%> (ø)
... and 18 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 5188824...9ffae69. Read the comment docs.

@nolanbconaway
Copy link
Member

yes! great call @pwildenhain !

@NiallRees
Copy link
Member

🎉

Copy link
Member

@alanmcruickshank alanmcruickshank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit on the badge. Otherwise I've skimmed though all of the changes and I think they're going to be quite useful. If nothing else it's reminded me how far this project has come!

README.md Outdated
@@ -12,6 +12,7 @@
[![Requirements Status](https://img.shields.io/requires/github/sqlfluff/sqlfluff.svg?style=flat-square)](https://requires.io/github/sqlfluff/sqlfluff/requirements/?branch=master)
[![CircleCI](https://img.shields.io/circleci/build/gh/sqlfluff/sqlfluff/master?style=flat-square&logo=CircleCI)](https://circleci.com/gh/sqlfluff/sqlfluff/tree/master)
[![ReadTheDocs](https://img.shields.io/readthedocs/sqlfluff?style=flat-square&logo=Read%20the%20Docs)](https://sqlfluff.readthedocs.io)
[![Code style: black](https://img.shields.io/badge/code%20style-black-000000.svg)](https://github.com/psf/black)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a nit, but can we add ?style=flat-square to the url here? That should give the badge the same square edges as all the others.

@pwildenhain
Copy link
Member Author

🤷‍♂️ No idea why code coverage suffered, but I believe this is good to go 🚢

Copy link
Member

@alanmcruickshank alanmcruickshank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go - Merging now 👍

@alanmcruickshank alanmcruickshank merged commit aa182d2 into master Oct 20, 2020
@pwildenhain pwildenhain deleted the try-out-black branch November 22, 2020 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt black coding style
4 participants