-
Notifications
You must be signed in to change notification settings - Fork 479
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
tests: Change tests/runtest.py to match the pylint code style #1810
Conversation
Thanks, but it looks too many changes are applied in a single patch. Is it possible to apply each pylint violation one at a single commit then create multiple commits? |
Simply speaking, it's very difficult to review if a single change contains too many different changes. |
Oh I get it. |
Can you please change the max line length from 80 to 100? |
Okay, I'll incorporate your suggestion into the code! Thanks |
changed the max line length from 80 to 100 Signed-off-by: Soyeon Park <[email protected]>
If you want to catch all exceptions that signal program errors, use ``except Exception:`` Signed-off-by: Soyeon Park <[email protected]>
Used when a variable is defined but not used. Signed-off-by: Soyeon Park <[email protected]>
Though there are unusual situations where these give different results. Signed-off-by: Soyeon Park <[email protected]>
c78d127
to
34a062c
Compare
The pylint warning you mentioned is left W0613 (unused-argument), |
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.
LGTM, and it'd be nice if you could share the exact command line you ran to get these warnings.
Oh, you already wrote the command line in the PR. I'll merge the change then. |
Ugh, please put a blank line after the subject line later so that I hate github displaying such a subject line without any warnings. |
@sypark9646, You will understand what @namhyung mentioned by running The commit message should have a separate commit title, which is written in a single line at the top. For example, the your commit message is written as follows.
But it should have a separate commit title in a single line. Because of this, the
So the commit message should be changed with an empty line as follows.
It will show the commit log with
This PR is already merged so we can't change it, but please keep that in mind for the next PR. Thanks. |
Hello, @sypark9646, In addition to Honggyu's comment, Thank you for your contribution. :D |
Ah, thank you. I'll make sure to keep that in mind next time!! |
Is there any checker doing this automatically? |
There is https://github.com/conventional-changelog/commitlint but I haven’t had a chance to add it in our pre-commit hook yet. |
apply
black
for linting, and follow common rules in python code.The following warnings from pylint are fixed.
If you want to find the full pylint warning, run the following command:
And if you want to find a specific pylint warning(ex. W0613), you can run a command like this:
resolves #1497
related #1396