Skip to content

Fix some flake8 coding style issues#10

Closed
gerardroche wants to merge 1 commit intopackagecontrol:masterfrom
gerardroche:flake-fixes
Closed

Fix some flake8 coding style issues#10
gerardroche wants to merge 1 commit intopackagecontrol:masterfrom
gerardroche:flake-fixes

Conversation

@gerardroche
Copy link
Contributor

This adds ignore rules for several specific docstring warnings in the
form of D*. Those warnings tend to create a lot of flake8 noise.

Several other warnings have been fixed.

This adds ignore rules for several specific docstring warnings in the
form of D*.  Those warnings tend to create a lot of flake8 noise.

Several other warnings have been fixed.
@coveralls
Copy link

coveralls commented Jun 1, 2017

Coverage Status

Coverage remained the same at 66.211% when pulling 199cbb7 on gerardroche:flake-fixes into 5a63084 on packagecontrol:master.

Copy link
Member

@FichteFoll FichteFoll left a comment

Choose a reason for hiding this comment

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

I'm generally not against linting for docstrings, but that only makes sense if you actually took the effort to write some. I didn't, or only for a select few functions, which is why don't consider docstring linting worth it for this project.

As such, I'm going to reject this pull request. I'll apply a few of your whitespace changes manually, though. Thanks for your efforts regardless.


def main():
"""\
"""Main.
Copy link
Member

Choose a reason for hiding this comment

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

This docstring is later used to generate the help message, which is sub-optimal but works. Your change introduced a random "Main." in the help message.

assert_none = not (failure_asserts or all_failure_asserts
or warning_asserts or all_warning_asserts)
assert_none = not (
failure_asserts or
Copy link
Member

Choose a reason for hiding this comment

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

I prefer having the binary operator on the new line to more clearly show its relation to the above line. I believe there is an error code for this that goes both ways, so I'll see if I can enable this check for the future.

# D103 Missing docstring in public function
# D104 Missing docstring in public package
# D105 Missing docstring in magic method
ignore = D100, D101, D102, D103, D104, D105
Copy link
Member

Choose a reason for hiding this comment

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

Flake8, by default, doesn't check docstrings. You must have a flake8 plugin installed in your system/environment.

Also refer to the list of default errors: http://flake8.pycqa.org/en/latest/user/error-codes.html

@FichteFoll FichteFoll closed this Jun 1, 2017
@FichteFoll
Copy link
Member

FichteFoll commented Jun 1, 2017

After experimenting with flake8-docstrings for a bit, this turned out to be not that bad of an idea, so I added it in c96f1f2.

Also quite unfortunately the error code for not having a line break before binary operators rather than after is still WIP: PyCQA/pycodestyle#502.

@gerardroche
Copy link
Contributor Author

Adding the ignore rules for the docstrings doesn't hurt, in fact it tends to hurt not to, unless I'm doing something wrong.

I use SublimeLinter and it automatically shows lots of docstring errors without those ignore rules. I'm sure there's a way to disable the docstring checks per project or enable them per project but I find it easier to add the ignore rules and fix the issues. Code usually conforms for the most part, at least without those few high noise rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants