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

Package linting #7

Merged
merged 3 commits into from
Oct 17, 2017
Merged

Package linting #7

merged 3 commits into from
Oct 17, 2017

Conversation

joshblum
Copy link
Contributor

@joshblum joshblum commented Oct 5, 2017

Depends on #6 (once that is merged I'll rebase this so it cleanly reflects only the lint changes). @kmax12 there are a lot of changes to make to make the project pass flake8.

$ flake8 featuretools | wc -l
     854

I cleaned the file featuretools/computational_backends/calculate_feature_matrix.py as an example. Let me know if you want me to proceed with adding linting to the build (and getting the other files to pass). If you have any preference for the flake8 config let me know before I continue.

@kmax12 kmax12 requested a review from rwedge October 5, 2017 22:41
@kmax12
Copy link
Contributor

kmax12 commented Oct 11, 2017

Thanks @joshblum. For the linting, let's turn off the 80 character limit check. For now, we will leave it to the developer to decide if going over 80 characters will increase the readability of the code.

For example, in your changes to calculate_feature_matrix.py, I think the 80 chars on the docstring is great for readability, but most of the other changes don't increase the readability of the code.

from . import tests
from .utils.pickle_utils import *
import featuretools.demo
import config # noqa
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can put # flake8: noqa at the top of the file rather than repeat on every line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good!

@joshblum joshblum force-pushed the package-linting branch 2 times, most recently from 3ca99f1 to 236be37 Compare October 11, 2017 17:44
@joshblum
Copy link
Contributor Author

@kmax12 @rwedge PR should be good to go. Let me know if you would like any changes! All builds will run flake8 and isort to check for any syntax/PEP8 errors/import order errors. I recommend adding autopep8 and an isort plugin to your editor to automatically apply these rules during development.

@joshblum joshblum force-pushed the package-linting branch 2 times, most recently from f747293 to 4b2f726 Compare October 12, 2017 01:23
@codecov-io
Copy link

codecov-io commented Oct 12, 2017

Codecov Report

Merging #7 into master will decrease coverage by 0.03%.
The diff coverage is 95.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
- Coverage   86.86%   86.83%   -0.04%     
==========================================
  Files          73       73              
  Lines        6778     6768      -10     
==========================================
- Hits         5888     5877      -11     
- Misses        890      891       +1
Impacted Files Coverage Δ
featuretools/computational_backends/__init__.py 100% <ø> (ø) ⬆️
featuretools/primitives/__init__.py 100% <ø> (ø) ⬆️
featuretools/synthesis/__init__.py 100% <ø> (ø) ⬆️
featuretools/utils/pickle_utils.py 75.86% <ø> (ø) ⬆️
featuretools/entityset/__init__.py 100% <ø> (ø) ⬆️
featuretools/core/api.py 100% <ø> (ø) ⬆️
featuretools/core/base.py 21.73% <ø> (ø) ⬆️
featuretools/variable_types/__init__.py 100% <ø> (ø) ⬆️
featuretools/demo/__init__.py 100% <ø> (ø) ⬆️
featuretools/tests/testing_utils/__init__.py 100% <ø> (ø) ⬆️
... and 66 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 fee8653...12a982c. Read the comment docs.

@kmax12 kmax12 merged commit ddba9c3 into alteryx:master Oct 17, 2017
@joshblum joshblum deleted the package-linting branch October 17, 2017 15:42
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.

3 participants