Skip to content
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.

Sign up for GitHub

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

Flake8 Template Change to Reduce Warnings #21

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Flake8 Template Change to Reduce Warnings #21

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9350f9e
Adapt stats collection to allow for colon
rnickle Nov 13, 2021
0762b68
Add extra field for column.
rnickle Nov 13, 2021
4cdda0c
Make sure group returns str not tuple
rnickle Nov 13, 2021
a86ca4a
Merge pull request #1 from rnickle/fewerwarnings
rnickle Nov 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions genbadge/utils_flake8.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from __future__ import division

from warnings import warn
import os
import re

from .utils_badge import Badge
Expand Down Expand Up @@ -104,7 +105,8 @@ def get_flake8_stats(flake8_stats_file):
return parse_flake8_stats(flake8_stats_txt)


RE_TO_MATCH = re.compile(r"([0-9]+)\s+([A-Z0-9]+)\s.*")
RE_TO_MATCH = re.compile(r"([0-9]+)\s+([A-Z0-9]+):*\s.*")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
RE_TO_MATCH = re.compile(r"([0-9]+)\s+([A-Z0-9]+):*\s.*")
# Note: the optional colon `:*` is to comply with some flake8 plugins, see PR #21
RE_TO_MATCH = re.compile(r"([0-9]+)\s+([A-Z0-9]+):*\s.*")

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for reviewing this @smarie. I don't see why the very small patch related to the colon shouldn't be merged.

To be certain that this different format was the exception rather than some new format, I just did a quick review of the three plugins I found that have the colon:

flake8-comments 0.1.2 Was last updated 5 months ago, no comments or PRs.
flake8-type-annotations 0.4.0 Is active with a few comments and PRs.

To my surprise:

flake8-type-annotations 0.1.0 Is actually DEPRECATED, because the functionality was merged into flake8 under different rules, so I will stop using this one.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't see why the very small patch related to the colon shouldn't be merged.

It should ! The suggestion I made is about adding a comment above the line. For some reason github does not highlight the diff correctly.
Thanks for investigating this list of colon-creating plugins

SOURCE_MATCH = re.compile(r"([^:]+):\d+:\d+:\s+.*")


def parse_flake8_stats(stats_txt # type: str
Expand All @@ -115,7 +117,14 @@ def parse_flake8_stats(stats_txt # type: str
for line in stats_txt.splitlines():
match = RE_TO_MATCH.match(line)
if not match:
Copy link
Owner

@smarie smarie Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Now that there are several cases, it would probably be preferable to first handle the "match" (reverting the if). That would put the nominal case upfront.

Each case could start with a comment illustrating the kind of pattern.
For the nominal one for example # Nominal case, such as: "N400: Found backslash that is used for line breaking"

Copy link
Author

Choose a reason for hiding this comment

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

Pardon me for the delay, I looked into it a little, and just had a chance to go back. I was concerned about a variety of things, including exactly how find_severity worked. I added my flake8stats.txt and modified the tests to make sure they could pass with it.

warn("Line in Flake8 statistics report does not match template and will be ignored: %r" % line)
smatch = SOURCE_MATCH.match(line)
Copy link
Owner

@smarie smarie Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
smatch = SOURCE_MATCH.match(line)
# Other supported lines formats ?
smatch = SOURCE_MATCH.match(line)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, thank you @smarie, I did not realize that it wasn't being caught because I did not audit and verify the counts.

I have quite a few warnings from several flake8 plugins. I can organize the ones you don't already have into tests/reports/flake8/flake8stats.txt and cross-check the counts. I will report back on my findings, and if I introduce any changes to the code I will provide tests.

It seems to me that I should also provide tests for the ":" cases, so I will try testing against that also. I will try to turn this around over the next few days.

Copy link
Owner

Choose a reason for hiding this comment

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

That would be great @rnickle ! My guess is that with one or two additional test files it should cover most of the variants, but I let you find investigate. Let me know !

if smatch:
source = smatch.group(1)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
source = smatch.group(1)
# Line with source file, such as "directory/sourcefile.py:139:33: Q000 Single quotes found but double quotes preferred"
source = smatch.group(1)

if not os.path.exists(source):
warn("Source report line does not refer to file: %r" %line)
else:
# warn on lines that do not match stats or source report.
warn("Line in Flake8 statistics report does not match template and will be ignored: %r" % line)
else:
nb, code = match.groups()
stats.add(int(nb), code)
Expand Down