sopel-irc / sopel

:robot::speech_balloon: An easy-to-use and highly extensible IRC Bot framework. Formerly Willie.
https://sopel.chat
Other
949 stars 403 forks source link

docs, dev-requirements: upgrade sphinx and try to fix some type issues with documentation #2496

Closed Exirel closed 11 months ago

Exirel commented 1 year ago

Description

First, I upgraded Sphinx to the latest version, which requires to upgrade Furo as well. That went well so that's good! Then I added links to the general index and the Python module index, because they are nice.

Then I tried to fix Sphinx autodoc's warnings regarding type annotations. I more or less figure out a way to fix one or two... but then I got stuck very hard: Sphinx, currently, does not handle type annotation properly. For some reason it works for TypedRule now but not with some datetime.datetime annotation? There are clearly bugs (that are not new to version 7.0 of Sphinx by the way), and I tried few options (such as using autodoc_type_aliases, or for a moment I switch to another autodoc extension, with the same issues). Sopel is not an isolated case, as this issue shows it.

So yeah, since it's mostly Sphinx's fault, I decided to give up on the remaining warnings. I don't think we can do much: we could, hypothetically, tinker for hours with the code... but I don't see the value here. I already spent quite a few hours on this and I'm not happy so let's not waste any more time.

Note: this won't work with Python 3.7 as I use a feature from Python 3.8 (typing.Protocol). So until @dgw publish his PR to remove Python 3.7 support, this will be a draft only.

Checklist

dgw commented 1 year ago

Was looking for something else (after someone asked a question in Furo's discussions) and noticed https://deploy-preview-2496--sopel-unstable-docs.netlify.app/package/db#sopel.db.SopelDB has a not-very-helpful annotation of a parameter:

image

(It's the <class 'sopel.tools.identifiers.Identifier'> notation. It's too verbose, and it's not even x-ref'd.)

Along the way to that, I also noticed some long signatures and remembered about sphinx-doc/sphinx#11011 adding a configuration setting (maximum_signature_line_length) in Sphinx 7.1.0 that makes long signatures multi-line, just like we do in the code (and Furo should handle it fine). Want to make that part of this patch, since you're upgrading Sphinx anyway?

dgw commented 1 year ago

Uh-oh. We'd better keep an eye on this. "Furo is broken on Sphinx 7.2" https://github.com/pradyunsg/furo/discussions/693

dgw commented 1 year ago

Quick responses! https://github.com/pradyunsg/furo/discussions/693#discussioncomment-6753443 (Furo fix) + https://github.com/pradyunsg/furo/discussions/693#discussioncomment-6753629 (Sphinx fix)

Exirel commented 1 year ago

Along the way to that, I also noticed some long signatures and remembered about sphinx-doc/sphinx#11011 adding a configuration setting (maximum_signature_line_length) in Sphinx 7.1.0 that makes long signatures multi-line, just like we do in the code (and Furo should handle it fine). Want to make that part of this patch, since you're upgrading Sphinx anyway?

Hell yeah, and it works!

dgw commented 1 year ago

Hmm, I wonder why solving the Sphinx requirement failed… There's a new Furo version (2023.9.10) you can use as an excuse to re-run CI 😁

SnoopJ commented 1 year ago

Hmm, I wonder why solving the Sphinx requirement failed…

I believe it's because the relevant requirement is sphinx<8,>=7.2.3, but Sphinx (surprisingly?) dropped 3.8 support in 7.2.0

dgw commented 1 year ago

surprisingly?

A bit. 3.8 isn't EOL until over a year from now (October '24), and it's also weird (to me) to drop a Python version in a minor bump.

That said, SemVer is meaningless… but oy. I guess we can probably get away with Sphinx 7.2.x for Python >=3.9 and Sphinx 7.1.x for Python 3.8. We Probably™ don't rely on any of the fixes/changes in Sphinx 7.2.0, and Furo should have finally sorted out all of its cross-version compatibility at this point.

I hope. 😅

dgw commented 1 year ago

@Exirel I looked at the changes since my last review and I think this is fine except for the failure on Python 3.8. Could you sort that out when you get a moment, so we can verify that the docs build correctly? Looks like Netlify didn't even try to make a deploy preview for the latest commit; there's no check annotation. ☹️

dgw commented 1 year ago

Before I re-review this one last time and give you the :shipit: squash instruction, are the Sphinx warnings in the build log for cdc9a10bbfd68c58d01028bcbdf4575d42b01837 part of what you were trying to fix?

Long Sphinx log snippet ``` 3:37:31 PM: /opt/build/repo/sopel/irc/modes.py:docstring of sopel.irc.modes.ModeMessage:1: WARNING: py:class reference target not found: ModeDetails 3:37:31 PM: /opt/build/repo/sopel/irc/modes.py:docstring of sopel.irc.modes.ModeMessage:1: WARNING: py:class reference target not found: PrivilegeDetails 3:37:31 PM: /opt/build/repo/sopel/irc/modes.py:docstring of sopel.irc.modes.ModeMessage:1: WARNING: py:class reference target not found: ModeTuple 3:37:31 PM: /opt/build/repo/sopel/irc/modes.py:docstring of sopel.irc.modes.ModeMessage.ignored_modes:1: WARNING: py:class reference target not found: ModeTuple 3:37:31 PM: /opt/build/repo/sopel/irc/modes.py:docstring of sopel.irc.modes.ModeMessage.modes:1: WARNING: py:class reference target not found: ModeDetails 3:37:31 PM: /opt/build/repo/sopel/irc/modes.py:docstring of sopel.irc.modes.ModeMessage.privileges:1: WARNING: py:class reference target not found: PrivilegeDetails 3:37:31 PM: /opt/build/repo/sopel/irc/modes.py:docstring of sopel.irc.modes.parse_modestring:1: WARNING: py:class reference target not found: ModeTuple 3:37:31 PM: /opt/build/repo/sopel/plugins/handlers.py:docstring of sopel.plugins.handlers.AbstractPluginHandler.get_capability_requests:1: WARNING: py:class reference target not found: plugin_decorators.capability 3:37:31 PM: /opt/build/repo/sopel/plugins/handlers.py:docstring of sopel.plugins.handlers.PyModulePlugin.get_capability_requests:1: WARNING: py:class reference target not found: plugin_decorators.capability 3:37:31 PM: /opt/build/repo/sopel/plugins/rules.py:docstring of sopel.plugins.rules.AbstractRule.is_channel_rate_limited:1: WARNING: py:class reference target not found: datetime 3:37:31 PM: /opt/build/repo/sopel/plugins/rules.py:docstring of sopel.plugins.rules.AbstractRule.is_user_rate_limited:1: WARNING: py:class reference target not found: datetime 3:37:31 PM: /opt/build/repo/sopel/plugins/rules.py:docstring of sopel.plugins.rules.Rule.is_channel_rate_limited:1: WARNING: py:class reference target not found: datetime 3:37:31 PM: /opt/build/repo/sopel/plugins/rules.py:docstring of sopel.plugins.rules.Rule.is_user_rate_limited:1: WARNING: py:class reference target not found: datetime 3:37:31 PM: /opt/build/repo/sopel/tools/identifiers.py:docstring of sopel.tools.identifiers.Identifier.casemapping:1: WARNING: py:class reference target not found: sopel.tools.identifiers.Casemapping 3:37:31 PM: /opt/build/repo/sopel/tools/memories.py:docstring of sopel.tools.memories.SopelIdentifierMemory.make_identifier:1: WARNING: py:class reference target not found: sopel.tools.identifiers.IdentifierFactory 3:37:31 PM: /opt/build/repo/sopel/tools/target.py:docstring of sopel.tools.target.Channel.make_identifier:1: WARNING: py:class reference target not found: IdentifierFactory 3:37:31 PM: /opt/build/repo/sopel/trigger.py:docstring of sopel.trigger.Trigger:1: WARNING: py:class reference target not found: Match ```

I ask because at least some of the reference targets not found are part of your diff, as aliases in docs/source/conf.py.

Exirel commented 1 year ago

Yeah, I tried, but as I said in the PR's description, for some reasons Sphinx just can't handle some cases, and I still don't understand why or how. I gave up on these. 😭

Exirel commented 1 year ago

All squashed and ready to go.