-
Notifications
You must be signed in to change notification settings - Fork 184
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
Fix PEP-8 violations reported by flake8 on asv folder #988
Fix PEP-8 violations reported by flake8 on asv folder #988
Conversation
.github/workflows/ci.yml
Outdated
- name: flake8 validating correct file(s) | ||
run: flake8 asv/step_detect.py asv/runner.py asv/results.py asv/repo.py asv/main.py |
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.
Are all the files in this PR added here? Seems like some are missing, is there a reason for it?
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.
Thank you, Marc. You are right! I'm going to include all the respective files.
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.
Looks good, added few comments. For next time, try to open smaller PRs.
asv/statistics.py
Outdated
@@ -387,7 +388,7 @@ def binom_pmf(n, k, p): | |||
logp = math.log(p) | |||
log1mp = math.log(1 - p) | |||
return math.exp(lgamma(1 + n) - lgamma(1 + n - k) - lgamma(1 + k) | |||
+ k*logp + (n - k)*log1mp) | |||
+ k*logp + (n - k)*log1mp) |
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.
Seems like spaces are missing around operators. Isn't flake8 complaining?
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.
It was my mistake. I'm working on fixing it!
asv/util.py
Outdated
@@ -531,7 +543,7 @@ def get_content(header=None): | |||
if env and WIN and sys.version_info < (3,): | |||
# Environment keys and values cannot be unicode | |||
def _fix_env(s): | |||
return s.encode('mbcs') if isinstance(s, unicode) else s | |||
return s.encode('mbcs') if isinstance(s, unicode) else s # noqa |
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.
Another space is missing before the hash. Also, we need the error we are ignoring after noqa.
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!
asv/util.py
Outdated
@@ -1127,9 +1147,9 @@ def format_text_table(rows, num_headers=0, | |||
|
|||
def _datetime_to_timestamp(dt, divisor): | |||
delta = dt - datetime.datetime(1970, 1, 1) | |||
microseconds = (delta.days * 86400 + delta.seconds) * 10**6 + delta.microseconds | |||
microseconds = ((delta.days * 86400 + delta.seconds) * 10**6 + delta.microseconds) |
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.
Spaces around the ** operator
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 spaces are not necessary, and flake8 doesn't point to it as a mistake.
CI is red, looks like you're breaking the tests. This PR is too big, maybe better to start by splitting it like in 3, one for util.py, one for step_detect.py which are quite large files, and one for the rest. This will help find the problem, as well as making it easier to review, even if it takes few iterations. |
@datapythonista Thank you for your advice. Now, this PR is shorter than before, and de CI is working! |
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.
Thanks @LucyJimenez, really nice.
We'll merge it after the release, but looks perfect.
Thanks @LucyJimenez |
Check flake8 and fix issues pep8 related on asv/asv folder for: