-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Travis CI: make lint-diff mandatory #3743
Travis CI: make lint-diff mandatory #3743
Conversation
# On pull requests, run all flake8 tests on modified code | ||
# if clause avoids `fatal: ambiguous argument 'origin/master'` on git push | ||
- if [ "$TRAVIS_EVENT_TYPE" == "pull_request" ]; then | ||
make lint-diff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is different; this runs the more aggressive flake8 against the diff, no? We still want that (unless our code has become compatible with the stricter flake8 config)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A failure of make lint-diff
does not currently fail the Travis CI test run.
There are two issues with the way that we are running make lint-diff
:
- The Travis build passes even when
make lint-diff
fails. - Flake8 is choking when handed binary data like .mrc files.
I believe that these two issues are easy to fix but we would start failing the build when contributors fix typos. I believe that we would be better served by blackening our code before we make make lint-diff
mandatory. If not, we will frustrate our contributors with rework. Manual rework may cause bugs. If we do not blacken first then we will spend a lot of code review time on educating contributors on code formatting rules related to parts of the lines that they do not wish to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's enable this + make it mandatory. I don't think it'll be that annoying, since it'll only complain about the lines people are editing.
To fix 2, would adding a --filename=*.py
to scripts/flake8-diff.sh do the trick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider blackening our code in the future, but not sure this is the best time considering how many things we have on our plate 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Let's give that a shot :)
Improved automated testing
allow_failures
mode.Technical
Testing
Screenshot
Stakeholders