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

tools: Overhaul docstrings #1563

Merged
merged 2 commits into from
Apr 22, 2019
Merged

tools: Overhaul docstrings #1563

merged 2 commits into from
Apr 22, 2019

Conversation

dgw
Copy link
Member

@dgw dgw commented Apr 21, 2019

It started from noticing some extra blank lines and a few typos, and snowballed into this reasonably large changeset. Sometimes I just can't stop fixing documentation once I start…

This might or might not get added onto. I only touched tools/__init__.py so far, and haven't looked at any of the subpackages. (web is safe until #1318 gets sorted—that branch is still WIP, but largely done.)

Officially marking the SopelMemory.contains() method (and its fraternal twin in SopelMemoryWithDefault) as deprecated felt really good. Can't wait to get rid of them for good…

@dgw dgw added this to the 7.0.0 milestone Apr 21, 2019
@dgw dgw requested a review from Exirel April 21, 2019 09:51
@dgw dgw force-pushed the sopel.tools-docstring-cleanup branch from d54c307 to 5129b60 Compare April 21, 2019 10:05
sopel/tools/__init__.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Show resolved Hide resolved
sopel/tools/__init__.py Show resolved Hide resolved
sopel/tools/__init__.py Show resolved Hide resolved
@dgw
Copy link
Member Author

dgw commented Apr 22, 2019

@Exirel Pushed all my changes based on your review without rebasing, so you can see them more easily. Also fixed a related conflict between the @deprecated decorator and how Sphinx builds docs for wrapped decorated functions, so their signatures will work now. Just have to figure out how I managed to break decorators I didn't even touch, now…

@dgw dgw force-pushed the sopel.tools-docstring-cleanup branch from ab857d7 to ffebe06 Compare April 22, 2019 08:17
@dgw
Copy link
Member Author

dgw commented Apr 22, 2019

Unrelated decorator breakage sorted, and it turns out we don't need that monkey-patch I added into Sphinx's config in a previous version of this branch. Good to re-review 👍

@Exirel
Copy link
Contributor

Exirel commented Apr 22, 2019

🚢

dgw added 2 commits April 22, 2019 03:55
It started from noticing some extra blank lines and a few typos, and
snowballed into this reasonably large changeset. Sometimes I just can't
stop fixing documentation once I start…
The way it worked before, Sphinx would output `**kwargs` as the function
parameters for methods decorated with `deprecated`, instead of the real
signature. Not surprising, I guess, given what the code was doing.

As for why we weren't already using functools.wraps in the decorator
itself, it's been around since the root commit of this repo, ten years
ago, back when this project was still named "Jenni". The functools
module was added in Python 2.5, but maybe this decorator was originally
added before that version came out? We might never know, but let's use
the modern way from now on!
@dgw dgw force-pushed the sopel.tools-docstring-cleanup branch from ffebe06 to bc98c38 Compare April 22, 2019 08:55
@dgw
Copy link
Member Author

dgw commented Apr 22, 2019

Squished. Waiting for Travis…

@dgw dgw merged commit adc5ad4 into master Apr 22, 2019
@dgw dgw deleted the sopel.tools-docstring-cleanup branch April 22, 2019 08:58
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.

2 participants