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

plugin: clean up redundant in-docstring parameter type annotations #2539

Merged
merged 4 commits into from
Nov 9, 2023

Conversation

dgw
Copy link
Member

@dgw dgw commented Nov 1, 2023

Description

I realized that leaving parameter types in the docstrings of these decorators is a maintenance nightmare waiting to happen. Our docs show the type-hints in the function signatures very nicely now. Putting types in the parameter list too is just redundant.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make lint and make test)
    • I mean there'd better not be any, since I didn't touch code.
  • I have tested the functionality of the things this change touches
    • By this I mean I test-built the docs locally. Sphinx still warns about some type annotations/x-refs that it has trouble resolving, but those aren't new (discussed in #2496 and variously on IRC).

@dgw dgw added this to the 8.0.0 milestone Nov 1, 2023
@dgw dgw requested a review from a team November 1, 2023 00:13
sopel/plugin.py Show resolved Hide resolved
@dgw dgw force-pushed the plugin-docstring-types branch from 78c21ee to d0e9e24 Compare November 1, 2023 05:56
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

Aside from a nitpick mentioned by SnoopJ, I really like that!

sopel/plugin.py Show resolved Hide resolved
@dgw dgw requested review from SnoopJ and Exirel November 1, 2023 15:31
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

Yeah, still LGTM!

@dgw
Copy link
Member Author

dgw commented Nov 2, 2023

I'm going to rebase with a few more tweaks ahead of @SnoopJ's rereview. Found some more things I could clean up, in particular some of the more confusing decorator type-hints (plugin no longer even imports typing.Any in my local copy 🥳).

I've definitely gone a little beyond that exact description in the name
of improving consistency, but that's the gist.
@dgw dgw force-pushed the plugin-docstring-types branch from fd84b3e to cec43ee Compare November 2, 2023 16:12
dgw added 2 commits November 2, 2023 11:16
The appearance of `Any` in the docs for plugin decorators bugged me so I
worked on the type annotations until it was no longer used in the file.
@dgw dgw force-pushed the plugin-docstring-types branch from cec43ee to 08b670c Compare November 2, 2023 16:20
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

Let's go.

@dgw
Copy link
Member Author

dgw commented Nov 9, 2023

13:20:57 <+dgw> SnoopJ, did you want to take another look at #2539 or should I just ship it?
13:21:38 <+SnoopJ> nah, ship it

@dgw dgw removed the request for review from SnoopJ November 9, 2023 19:22
@dgw dgw merged commit 89fec76 into master Nov 9, 2023
8 checks passed
@dgw dgw deleted the plugin-docstring-types branch November 9, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants