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.

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

Updates to satisfy pep257 violations #2028

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

Conversation

radez
Copy link
Contributor

@radez radez commented Apr 4, 2024

What kind of change does this PR introduce?

  • tests/coverage improvement

CI has a pep257 check in it that is riddled with violations.
This patch is an attempt to clean those up to help with enforcing new PRs pep257 compliance.

@radez
Copy link
Contributor Author

radez commented Apr 4, 2024

There are lots more violations, Just posting this so folks can comment or help.

@webknjaz

This comment was marked as resolved.

@radez radez force-pushed the pep257_fixes branch 3 times, most recently from 426edaa to c7f0ce6 Compare April 22, 2024 23:43
@webknjaz
Copy link
Member

@radez wow, that's a lot of work! I'll have to dedicate some time to reviewing it more carefully.

Meanwhile, since you seem to have addressed all of the rule violations, could you look into removing the exclusions?

There's a pattern for temporarily excluded files in the pydocstyle entry of .pre-commit-config.yaml — if I'm right, that entire exclude thing can be just removed.
And with that, we could delete the separate .pre-commit-config-pep257.yaml config — it only existed to run an indicative check in CI on all of the files that were excluded from the check that contributes to the overall CI outcome. With the removal of exclude in the main config, all the checks will be covered by that, so the pep257 job should also be removed from the CI as well as the pre-commit invocation (the entire env) should be deleted from tox.ini.

The next step could be replacing a separate pydocstyle check with a flake8-docstrings plugin — but that has a pre-requisite of making a few updates to .pre-commit-config.yaml, including version bumps so I'd postpone it until after this PR is merged.

cherrypy/_cperror.py Outdated Show resolved Hide resolved
cherrypy/_cplogging.py Outdated Show resolved Hide resolved
cherrypy/_cpdispatch.py Outdated Show resolved Hide resolved
cherrypy/_cpdispatch.py Outdated Show resolved Hide resolved
cherrypy/_cpdispatch.py Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

could you look into removing the exclusions?

Did that in 17b7f6d.

cherrypy/_cpcompat.py Outdated Show resolved Hide resolved
cherrypy/_cpcompat.py Outdated Show resolved Hide resolved
cherrypy/_cpcompat.py Outdated Show resolved Hide resolved
cherrypy/_cpdispatch.py Outdated Show resolved Hide resolved
cherrypy/_cpdispatch.py Outdated Show resolved Hide resolved
cherrypy/_cpdispatch.py Outdated Show resolved Hide resolved
cherrypy/_cpdispatch.py Outdated Show resolved Hide resolved
cherrypy/_cpdispatch.py Outdated Show resolved Hide resolved
cherrypy/_cpdispatch.py Outdated Show resolved Hide resolved
cherrypy/_cpdispatch.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
cherrypy/_cptools.py Outdated Show resolved Hide resolved
@@ -54,33 +53,36 @@ class VirtualHost(object):

cherrypy.tree.graft(vhost)
"""
default = None
"""Required.
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd still be nice to have actual class attribute docstrings, not just initializer args.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't disagree with you, there was just enough changes to make that I didn't put the time into having actual attribute docstrings

Copy link
Member

Choose a reason for hiding this comment

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

Yeah.. It might've been easier to merge smaller PRs. I got to review a part of this yesterday, during the sprints, patching it file-by-file. But didn't get through all the files — people around needed help with other stuff they were sprinting on. I was hoping to keep going through it today.

radez and others added 6 commits May 23, 2024 10:10
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
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.

None yet

2 participants