🎨 Apply flake8 for code style #1019
Conversation
stevenbal
force-pushed
the
feature/add-flake8
branch
2 times, most recently
from
86481a2 to
489d817
Compare
|
I'm not really in favor about style enforcing with flake8, I don't think catching unused variables and ordering imports improves a codebase sufficiently that it's worthwhile to mandate it via CI. I think black is sufficient. But I'm also not about enforcing my preferences, so if @Bartvaderkin and @pi-sigma are in favor we can merge this in |
|
@alextreme I'm down with this. Looking at the files changed, there are quite a few superfluous variable assignments and unused imports being cleaned up, f-strings without anything interpolated being converted to regular strings etc. I don't think this is merely about style. Whether lines should be 80 or 100 chars long is a matter of style and preference, but unused imports and variable assignments are just cluttering the namespace; no one would prefer to keep them if it was pointed out. Other things are perhaps debatable. See the below comment about unused imports in |
| log | ||
| for log in logs | ||
| if self.zaak1["identificatie"] in log.extra_data["message"] |
There was a problem hiding this comment.
Maybe not the best example but I want to be able to use single letter variables in cases like this. (ok maybe not lowercase l but in general)
| @@ -232,7 +230,7 @@ def get_absolute_url(self, category=None): | |||
| ) | |||
|
|
|||
| def has_cta_tag(self): | |||
| return "\[CTABUTTON\]" in self.content | |||
| return "\[CTABUTTON\]" in self.content # noqa | |||
There was a problem hiding this comment.
Due to src/open_inwoner/pdc/tests/test_product.py:271:46: W605 invalid escape sequence '\]'
There was a problem hiding this comment.
I wonder what this is supposed to be: does it even need the backslashes or was this a remnant of a regex?
There was a problem hiding this comment.
I'm not sure, but I didn't want to change the code without knowing whether it's still necessary
There was a problem hiding this comment.
This would be good to add, especially for the unused imports.
The amount of noqa's seems a bit much, and there a some that feel weird (I marked some)
Removing the unused variable names as hard rule is debatable, especially in tests they can help a lot showing what certain factories are for (I marked some examples). If we really must have this rule then restore the missing information (put it in a comment or some field on the object)
Also I want to keep the option to use single letter variables, sometimes they are more readable then breaking the whole thing up in a black mess, so let's leave that up to code review instead of a dogmatic rule.
|
@Bartvaderkin I agree, the main reason I added this was for the imports. I could add F841 (unused variables) and E741 (ambiguous/short variable names) to the ignore list. |
stevenbal
force-pushed
the
feature/add-flake8
branch
from
489d817 to
2407c6b
Compare
stevenbal
force-pushed
the
feature/add-flake8
branch
from
2407c6b to
99a4ec1
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1019 +/- ##
===========================================
+ Coverage 94.74% 94.78% +0.04%
===========================================
Files 872 871 -1
Lines 30546 30518 -28
===========================================
- Hits 28940 28926 -14
+ Misses 1606 1592 -14 ☔ View full report in Codecov by Sentry. |
|
@stevenbal Let's allow the short variables. I'm ambivalent about the unused variables rule but we if use it then let's convert the useful names from the tests into a comment. |
|
I added the unused variables and short variables rules to the ignorelist |
Better late than never :)