-
Notifications
You must be signed in to change notification settings - Fork 104
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
Adding pre commit #220
Adding pre commit #220
Conversation
3155b23
to
02eb87b
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #220 +/- ##
=======================================
Coverage 96.18% 96.18%
=======================================
Files 9 9
Lines 498 498
Branches 94 94
=======================================
Hits 479 479
Misses 9 9
Partials 10 10
|
tests/test_encoding.py
Outdated
self.assertIsNotNone(bom_encoding) | ||
self.assertIsNotNone(bom) |
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 change is breaking mypy, because mypy does not understand self.assert*.
If flake8 complains about asserts, I believe we need to disable that check for all files (we will also use asserts outside tests for mypy’s sake).
I am a bit puzzled because in Scrapy we use asserts often, but I see nothing about ignoring such an error in https://github.com/scrapy/scrapy/blob/master/.flake8 (the ignored errors seem unrelated).
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 made that change to avoid a bandit issue. It was something like:
>> Issue: [B101:assert_used] Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
Severity: Low Confidence: High
CWE: CWE-703 (https://cwe.mitre.org/data/definitions/703.html)
Location: tests/test_url.py:1063:8
More Info: https://bandit.readthedocs.io/en/0.0.0/plugins/b101_assert_used.html
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.
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.
Excluding tests from bandit checks would also be useful in any repo.
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.
Done in 5390b23
4a142a8
to
99bee2d
Compare
Adding config files first. If I get an OK I will proceed to execute the hooks on all the code and fix it to comply.