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

Add black linting package and remove autopep8 #2306

Merged
merged 18 commits into from Jun 3, 2021
Merged

Add black linting package and remove autopep8 #2306

merged 18 commits into from Jun 3, 2021

Conversation

angela97lin
Copy link
Contributor

@angela97lin angela97lin commented May 26, 2021

Closes #1917

Notes here: https://alteryx.quip.com/kmd8Aum8fsnF/Notes-on-using-the-Black-linting-package

Relevant files to look at:

  • Makefile
  • setup.cfg
  • dev-requirements.txt

I updated our isort version to 5.0.0 (could be anything beyond) which was the biggest isort release for a while. The most noteable differences for us are:

  • __init__ files are no longer ignored by default. Added configuration to ignore, since that causes circular dependencies.
  • we can use the black profile config to make the two compatible without adding additional configurations for either :)
  • isort no longer uses the --recursive flag; it automatically goes through directories recursively

Note that following discussion with team, we will hold merge of this PR until after the next release (post 0.25.0), but putting this up for review now so that everyone has a chance to look at the changes!

@angela97lin angela97lin self-assigned this May 26, 2021
@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #2306 (bda3099) into main (fbea545) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2306   +/-   ##
=====================================
  Coverage   99.9%   99.9%           
=====================================
  Files        281     281           
  Lines      24607   24607           
=====================================
  Hits       24578   24578           
  Misses        29      29           
Impacted Files Coverage Δ
evalml/data_checks/data_check_message_type.py 100.0% <ø> (ø)
evalml/exceptions/__init__.py 100.0% <ø> (ø)
evalml/objectives/lead_scoring.py 100.0% <ø> (ø)
evalml/pipelines/__init__.py 100.0% <ø> (ø)
...ml/pipelines/multiclass_classification_pipeline.py 100.0% <ø> (ø)
evalml/preprocessing/__init__.py 100.0% <ø> (ø)
evalml/tests/automl_tests/test_automl.py 99.7% <ø> (ø)
evalml/tests/component_tests/test_components.py 100.0% <ø> (ø)
.../tests/component_tests/test_datetime_featurizer.py 100.0% <ø> (ø)
...s/component_tests/test_decision_tree_classifier.py 100.0% <ø> (ø)
... and 343 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 fbea545...bda3099. Read the comment docs.

Makefile Outdated
@@ -12,7 +12,7 @@ lint:

.PHONY: lint-fix
lint-fix:
autopep8 --in-place --recursive --max-line-length=100 --select="E225,E222,E303,E261,E241,E302,E203,E128,E231,E251,E271,E127,E126,E301,W291,W293,E226,E306,E221" evalml
black .
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noteable change that isn't related to black's automagic cleanup.

@@ -4,8 +4,10 @@ filterwarnings =
ignore::DeprecationWarning
ignore::PendingDeprecationWarning
[flake8]
max-line-length = 88
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is also relevant to changes :)

This is also to make black compatible to flake8, see "Line Length": https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Where does W503 come from? I can't see it in the link you shared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Very cool!

@@ -2,5 +2,6 @@
-r test-requirements.txt
-r docs-requirements.txt
flake8==3.7.0
autopep8==1.4.3
black==21.5b1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Latest black version

setup.cfg Outdated
@@ -15,3 +17,8 @@ test=pytest
[isort]
forced_separate=evalml
multi_line_output=3
include_trailing_comma = True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make black compatible with isort, as specified here: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html

(See A compatible .isort.cfg``)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, did you also try this? https://pycqa.github.io/isort/docs/configuration/black_compatibility/

Looks like we can set profile=black?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, looks like we need isort >=5. Wonder if it's worth upgrading to not have to worry about the config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooo, I didn't know about this--this is really cool!! Looks like isort 5.0.0 was a big change (https://github.com/PyCQA/isort/blob/main/CHANGELOG.md) but I'd be down to try and see what changes it might cause / whether or not we like them :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #2334 to see the changes, looks like the main change is that submodules are sorted alphabetically, which is quite nice. I'd say it's a good idea to upgrade!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @angela97lin ! I agree!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, it looks like we have to ignore the init.py files though, since otherwise we run into import errors (circular dependencies and all that good stuff). Makes me wonder if we have the proper structure for all the init files but adding skip=__init__.py for the isort config for now.

@angela97lin angela97lin changed the title [SPIKE] Add black linting package Add black linting package and remove autopep8 May 27, 2021
Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

@angela97lin Looks good to me!

@@ -91,7 +92,9 @@ def submit_scoring_job(self, automl_config, pipeline, X, y, objectives):
"""Submit job for pipeline scoring."""


def train_pipeline(pipeline, X, y, optimize_thresholds, objective, X_schema=None, y_schema=None):
def train_pipeline(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think putting the arguments on a new line during function definition is the one stylistic thing that looks weird to me 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same 😂 I think this is done because the line gets too long aka too many arguments but I feel ya

except ImportError:
return 'Undersampler'
return "Undersampler"
Copy link
Contributor

Choose a reason for hiding this comment

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

Also surprised we don't have coverage here 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do! You probably saw it in the state where one of the linux tests hadn't passed: https://app.codecov.io/gh/alteryx/evalml/compare/2306/tree/evalml/automl/utils.py

Copy link
Contributor

Choose a reason for hiding this comment

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

You're totally right!

X = X.ww.select(include=["Integer", "Categorical"])

check_all_unique = X.nunique() == len(X)
cols_with_all_unique = check_all_unique[
Copy link
Contributor

Choose a reason for hiding this comment

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

This line split is pretty wild imo 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed 😂 this only happens because line length too long with comment, but I do like the inline comment more than having the comment above/below sigh

Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

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

LGTM! I like how this normalizes a lot of the formatting we do.

@angela97lin angela97lin merged commit c7a2990 into main Jun 3, 2021
@angela97lin angela97lin deleted the 1917_black branch June 3, 2021 22:08
@angela97lin angela97lin added this to the One Week Sprint milestone Jun 8, 2021
@chukarsten chukarsten mentioned this pull request Jun 9, 2021
@chukarsten chukarsten mentioned this pull request Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the "black" linting package in our lint CI
3 participants