Skip to content

Conversation

@gravesm
Copy link

@gravesm gravesm commented Aug 7, 2018

Closes #40. This adds a tox environment for flake8 and a corresponding
entry to the travis test matrix. Also fixes all the flake8 errors.

Closes #40. This adds a tox environment for flake8 and a corresponding
entry to the travis test matrix. Also fixes all the flake8 errors.
@gravesm gravesm requested review from JPrevost and hakbailey August 7, 2018 20:33
.where(func.upper(dlcs.c.ORG_HIER_SCHOOL_AREA_NAME).in_(AREAS)) \
.where(persons.c.PERSONNEL_SUBAREA_CODE.in_(PS_CODES)) \
.where(func.upper(persons.c.JOB_TITLE).in_(TITLES))
.where(func.upper(persons.c.JOB_TITLE).in_(TITLES)) # noqa: E711
Copy link
Member

Choose a reason for hiding this comment

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

🙈

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, this is required due to how SQLAlchemy is handling the operator overloading for things like some_column != None. pep8 says the comparison should be some_column is not None but this will not generate valid SQL.

Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

This is obviously a fine solution, but I think we should discuss as a team whether something like CodeClimate is preferred so we can normalize repository linting when possible.

I won't make my argument here as it's not specific to this PR ;)

:shipit:

@gravesm
Copy link
Author

gravesm commented Aug 8, 2018

I don't have a problem with also adding CodeClimate as another check during the Travis build. Feel free to submit a PR. However, I'm going to continue to insist on also using flake8 because it provides more checks than what CodeClimate will do. flake8 is a wrapper around pycodestyle, which is what CodeClimate runs, but it will also do additional checks for things like unused imports.

@gravesm gravesm merged commit 589c368 into master Aug 8, 2018
@gravesm gravesm deleted the flake8 branch August 8, 2018 14:13
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.

Add flake8 to tests

2 participants