First pass of docstring additions.

[nsnw]

Hey! With reference to #1565, I've started tidying up some of the docstrings in bot.py. I'm happy to help do others as well, but this is just to check that this is the kind of thing you're looking for. Feedback is obviously requested and will be gladly received!

[dgw]

I'm surprised and happy to see a contribution toward #1565 so soon! Thank you for jumping in!

First feedback round! 🚀 The takeaways are:

• More intersphinx links (see bot.write for an example of linking to the relevant glossary term for an argument's "type" when it's a concept, not a class or builtin)
  • Also intrasphinx (just normal references to functions, classes, constants… literally anything you can link to, inside or outside the Sopel codebase, should be linked)
• Consistent whitespace (no blank lines between method docstring and method body)
• Text wrapping
• Parameter/body ordering

Those last two aren't addressed in any line notes, so I'll explain.

In the overhaul so far (just #1563 at this point), we have:

1. Short method description (one line, ideally <80 chars)
2. Parameters & types, with return value if applicable
3. Longer description of the method, notes, etc.
4. Version notes (added, changed, deprecated, what-have-you)

This PR adds the parameters and their types at the end of the docstring. Neither way is necessarily better—Sphinx puts the parameter list at the end in its output regardless of where it appears in the source—but we should be consistent. (This is where I wonder if @Exirel has any strong feelings either way. I like having the parameters above the detailed notes, but if there are any reasons to prefer one or the other besides "how the source code looks", I'm all ears eyes.)

Additionally, the overhaul so far has wrapped parameter descriptions to a hanging indent:

:param foobar: a very long parameter description that requires wrapping
               to fit within a reasonable line length

This PR uses the following, different, style:

:param foobar: a very long parameter description that requires wrapping
    to fit within a reasonable line length

I personally find the hanging-indent style to be easier for reading in source code form, but I acknowledge that it can significantly constrain the available text width for parameters with long names. (We seem to fall into using :type foobar: a lot, too, because of all the classnames referenced, which we want to link—and that can't be done inline as :param :class:`sopel.tools.Identifier` foobar:.)

I was also loosely restricting my updates in #1563 to fit all docstring text within the 80-column limit of PEP8, because we do want to unignore those linter rules at some point in The Distant Future (The Year Two-Thousand).

These are all just my personal opinions based on what I already worked on. All are open for discussion, as this initiative is still very new, and we definitely should figure out as much of our "style" as possible before too many PRs get merged and it becomes more and more tedious to change direction mid-stream. Reformatting stuff we already reformatted is not fun!

Fair warning: I'm also cutting my time working on this a bit short, because it's almost 3am already (holy crap, how?!) and I need to sleep! But I wanted to get as much as I could typed out so the discussion can at least percolate a bit—if not move forward without me for a while, between you and other contributors—until I can come back to it.

Thanks again for hopping to it (not an Easter pun, I swear…) so soon after I opened that tracking issue! 🙌 🖖

[dgw]

I had all but given up trying to figure out how to link re.Pattern as an example, so I left it out of my review.

But! This worked when I tweaked the current PR state locally and built the HTML docs:

    def register_url_callback(self, pattern, callback):
        """Register a ``callback`` for URLs matching the regex ``pattern``

        :param pattern: compiled regex pattern to register
        :type pattern: :ref:`re.Pattern <python:re-objects>`
        :param function callback: callable object to handle matching URLs
[... etc ...]

pattern's type then links to https://docs.python.org/3/library/re.html#re-objects by doing this, which is precisely the section needed. Why it's not linkable as #re.Pattern despite describing what appear to be objects of that type, I don't know. But this is a solution to the "More intersphinx links" point I requested.

[nsnw]

Thanks for the feedback! I'll make a start on incorporating the feedback :-D

[nsnw]

I've incorporated the feedback, and:-

• docstrings are now limited to 80 characters wide
• moved parameter definitions to the top of the docstrings (under the initial one-line description)
• switching to hanging indentation for line-wrapped parameter definitions
• added glossary links for concepts (using :term: as suggested)
• removed blank newlines between docstring definitions and method bodies

[dgw]

Round two! Not a whole lot here, honestly. The indentation fixes and added links look 👌!

After this round is addressed, I'll pull the PR branch down and test-render it before (hopefully) final approval.

[dgw]

OK, this time I really think I caught everything. Honestly, I probably still missed an inconsistency or two, but let me know if you're tired of the back-and-forth. 😄

I drove away a contributor last year by being too demanding, and I really want to avoid doing that again… 😞

[nsnw]

You're not being too demanding!

I've pushed the latest version. I've added the "to a user or channel" wording to all the ones that it makes for now. I personally can't decide which is better - with or without it - but I'll leave it to you to decide.

[dgw]

You're not being too demanding!

That's a relief! All the same, I thank you for putting up with the lengthy iteration process. 😸

I've added the "to a user or channel" wording to all the ones that it makes for now. I personally can't decide which is better - with or without it - but I'll leave it to you to decide.

Obviously, I choose to keep "to a user or channel" in places where it makes sense to include, but without the second "a" that (to me) makes the phrase slightly redundant.

[dgw]

Phew. This project so far has taught me why the documentation I've seen even in large, well-established projects can be so inconsistent. It's exhausting making sure everything is "perfect", and time-consuming!

It appears that I was very wrong in my previous review earlier tonight, sadly. I didn't catch anywhere close to everything. Therefore, even though I think this is the real last batch of fixes, I'm not going to say it—maybe if I don't assert that this is the end, I'll avoid jinxing it?

[nsnw]

I am still working on this! I've just been busy with work. Hoping to get this finished in the next few days.

[Exirel]

Just a quick note to say: I love it! 👍
Also you'll have to rebase and resolve some conflict. And the more PR get merged, the more conflicts you'll have to fix.

[nsnw]

Ok! Sorry for the delay - currently in the process of buying a house so things got in the way -_-

In the latest commit I've implemented the suggestions, including:-

• switching single backticks to double backticks where appropriate - a scan of the generated HTML after this doesn't show any instances of <cite>/</cite>
• added periods at the end of sentences - I think I've now caught them all
• removed 2 instances of double spaces after a period

There were a couple of other bits I've found as well that I fixed.

[nsnw]

I've merged in the latest changes from master, so this should be current against those.

[dgw]

I've run out of nitpicks!

Besides the last few tweaks suggested, I would request that you clean up the branch commit history per our contribution guidelines. There's no "one right way" to do this, but in general we like to avoid merging fixup commits without squashing them first. This PR does not need to be squashed all the way down to a single commit, but there are a lot of "fix" messages in the log right now. :)

Apologies for the delay, @nsnw; I was traveling for the last three weeks with very little free time for this (or any) GitHub project.

[nsnw]

Excellent! Been a bit busy (getting married in a few weeks!) but I'll make the tweaks above and tidy up the PR. Once that's done I'll start looking at others :-)

[Exirel]

@nsnw that's lovely! 😍

[nsnw]

Okay - I believe I've squashed everything down to a single commit now. I have to admit that squashing and rebasing is not my strong point so if I've butchered it I apoplogise!

[Exirel]

🚢 (and don't wait too long because merge conflict can come fast with such changes!)

[dgw]

Great, let's .

@nsnw Congrats on the merge (momentarily)—but also, congratulations on the upcoming wedding!