Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

add support for sphinx-style parameters to D417 #595

Merged

Conversation

benji-york
Copy link
Contributor

Hi!

Numpy and Google-style parameter documentation is great, but some people use Sphinx-style parameter docs. This PR adds support for those to the D417 checker.

This is my first contribution to pydocstyle, so I'm unsure about the testing style. In particular, the tests I added in src/tests/test_sphinx.py seem to be a little out of character for the code base. However, they seem like good tests to me, so I would especially appreciate feedback on those.

--

Thanks for submitting a PR!

Please make sure to check for the following items:

  • Add unit tests and integration tests where applicable.
    If you've added an error code or changed an error code behavior,
    you should probably add or change a test case file under tests/test_cases/ and add
    it to the list under tests/test_definitions.py.
    If you've added or changed a command line option,
    you should probably add or change a test in tests/test_integration.py.
  • Add a line to the release notes (docs/release_notes.rst) under "Current Development Version".
    Make sure to include the PR number after you open and get one.

Please don't get discouraged as it may take a while to get a review.

@benji-york benji-york mentioned this pull request Jun 18, 2022
@benji-york
Copy link
Contributor Author

Can I get a maintainer to approve the workflow to run the CI checks?

Thanks!

@rrwalton
Copy link

Anything holding this up? Seems to have been open for a while. Would be great to get this feature in.

@rrwalton
Copy link

Anything holding this up? Seems to have been open for a while. Would be great to get this feature in.

Not meant to be a criticism in any way. Not sure why I got a thumbs down for this comment. I'd just find this feature useful, that's all.

@Pierre-Sassoulas
Copy link
Member

@benji-york, LGTM, but I can't approve or merge anymore despite #575. Please be patient @samj1912 will get to it at some point :)

@@ -35,6 +35,7 @@ New Features
* Add support for `property_decorators` config to ignore D401.
* Add support for Python 3.10 (#554).
* Replace D10X errors with D419 if docstring exists but is empty (#559).
* Add support for Sphinx-style parameter descriptions to D417.
Copy link
Member

Choose a reason for hiding this comment

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

@benji-york could you please move this to the latest version?

Copy link

@tomghc tomghc Jun 27, 2023

Choose a reason for hiding this comment

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

@benji-york any objections if I have a go at updating this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benji-york any objections if I have a go at updating this?

That'd be great, @tomghc!

(Sorry, I let this fall off my radar. If I can be of any service, let me know.)

Choose a reason for hiding this comment

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

@benji-york Hey, I was looking to help finish updating this to the current version so it could be pushed. Is there any way I could aid in development?

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'm happy for anyone to do anything on this. I'm not sure what @samj1912 means above by "move this to the latest version". Perhaps rebase?

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 kicked off an automated rebase, but the workflows are awaiting maintainer approval.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The automated rebase was successful. All tests passed. Now the review status has been reset, so this needs another review by a maintainer and if still acceptable (which it should be because nothing has changed), it needs merging by a maintainer.

Choose a reason for hiding this comment

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

Sounds great! Apologies for any inconvenience that I may have caused. I appreciate the work you put in!

@benji-york benji-york force-pushed the benji/add-sphinx-style-parameters branch from ab1f974 to 1e7dda2 Compare September 22, 2023 13:19
@benji-york
Copy link
Contributor Author

Meta comment: I'm not sure what constraints this project is running under, but the combination of required CI jobs for PR progress, required admin approval for CI jobs, and slow admin approval make for a painful contribution process—as exemplified by the timeline of this PR.

@sigmavirus24 sigmavirus24 merged commit 6d5455e into PyCQA:master Sep 22, 2023
15 checks passed
@JulianJvn
Copy link

This PR added the change to the changelog section about 6.2.0, which was released ten months before merging this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants