Skip to content
This repository has been archived by the owner on Sep 12, 2022. It is now read-only.

Make linter significantly more strict #664

Merged
merged 466 commits into from
Sep 12, 2018

Conversation

cdosborn
Copy link
Contributor

@cdosborn cdosborn commented Sep 7, 2018

Description

Increase linter from "low" to "medium"

Each commit fixes a single lint violation. I don't expect a reviewer to look at each commit. There are only a handful of commits that change more than one line, and they all fell into the same category of issue relating to imports.

File A, would from foo import X, but A would never use X at all. So the linter would suggest to remove that import. However file B, would import X from A. So file B would have to be updated to also import from foo. A file should import a value from the package where the value is defined.

Checklist before merging Pull Requests

  • Add an entry in the changelog
  • Reviewed and approved by at least one other contributor.

@cdosborn cdosborn force-pushed the lint-medium-strictness branch 2 times, most recently from a5f7ff3 to 4944f63 Compare September 10, 2018 23:05
@cdosborn cdosborn self-assigned this Sep 10, 2018
@cdosborn cdosborn force-pushed the lint-medium-strictness branch 2 times, most recently from cbfa908 to e1aa39e Compare September 10, 2018 23:25
@cdosborn cdosborn changed the title WIP Make linter quite a bit more strict Sep 10, 2018
@cdosborn cdosborn changed the title Make linter quite a bit more strict Make linter significantly more strict Sep 10, 2018
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 37.688% when pulling e1aa39e on cdosborn:lint-medium-strictness into 3348983 on cyverse:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 37.681% when pulling e1aa39e on cdosborn:lint-medium-strictness into 3348983 on cyverse:master.

@coveralls
Copy link

coveralls commented Sep 10, 2018

Coverage Status

Coverage decreased (-0.1%) to 37.864% when pulling 12fed6e on cdosborn:lint-medium-strictness into 47822d4 on cyverse:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 37.681% when pulling e1aa39e on cdosborn:lint-medium-strictness into 3348983 on cyverse:master.

@cdosborn cdosborn force-pushed the lint-medium-strictness branch 2 times, most recently from 8ca45da to 5d084a2 Compare September 11, 2018 19:54
@calvinmclean
Copy link
Contributor

Any tips for reviewing 250+ commits? 😱

@cdosborn
Copy link
Contributor Author

lol, at the top you'll see there are 464. I don't expect you to review each one. Look at the handful of commits that touch more than one file (or more than one line), maybe try and understand those. Or look at the commit messages and find any linting errors you don't understand and look at those.

api/base/views/__init__.py Show resolved Hide resolved
api/v1/views/meta.py Show resolved Hide resolved
service/instance.py Show resolved Hide resolved
Copy link
Contributor

@calvinmclean calvinmclean left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying all of my questions! Looks good and I am excited to have cleaned up code to work with

@cdosborn cdosborn merged commit 090e14e into cyverse:master Sep 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants