🔄 synced file(s) with WordPress/openverse

[openverse-bot]

synced local file(s) with WordPress/openverse.

Changed files

• synced local .pre-commit-config.yaml with remote templates/.pre-commit-config.yaml.jinja

---

This PR was created automatically by the repo-file-sync-action workflow run #4069993342

[AetherUnbound]

I'm working on addressing the docstring linting.

[AetherUnbound]

I made all the necessary docstring changes 👍🏼

[sarayourfriend]

Thank you for undertaking this Sisyphean task, Madison.

I haven't reviewed all the changes. I honestly hope I (and no one else on the team) don't have to because my recommendation is for us to scrap this linter. I was sceptical in the original issue that it would be a useful tool for us due to how contrived some of the rules are. I was able to get it to a somewhat reasonable state for the WordPress/openverse directory. Seeing it now applied to a larger Python codebase, I am struck by how much useless and absurd noise it forces us to add.

There are a few rules we could disable to ease some of these things. But my biggest gripes are all about the requirement of a "summary" line and there is no way to disable it, not even selectively like just disable it for modules.

I like the imperative mood rule and some other consistency things (especially adding whitespace, etc). I wish pydocstyle allowed disabling required summaries. As it stands, I think it is a bad idea to spend more time trying to make this work for our projects and forcing contributors to add senseless, noisy, useless pieces of documentation that invariably hurt the contribution experience, both for the person being forced to write useless documentation and for the people who have to learn to ignore them like banner ads in the 2000s.

I've gone ahead and approved it, and we can merge it because I figure the worst case is that we can revert this. Merging it now will at least prevent someone from having to re-do this absurd effort in the future if we decide to keep this tool.

[sarayourfriend]

This is really tedious to have to include 😞 I don't understand why pydocstyle requires a summary for __init__.

Maybe we should consider ignoring D402, which I believe is what forces this?

[sarayourfriend]

This and the "License utilities." one makes me question the entire endeavour. This kind of thing is the epitome of useless documentation and a linter that forces us to add them feels... like a bad linter.

[sarayourfriend]

This is yet another silly change to be forced to make 😢

[dhruvkb]

It might be a difference in the style of documentation but the API docs mostly use sentences and so this comment there would've been "This is a message builder for Slack.", with the full-stop being there by default due to the sentence-like nature of the comments.

[AetherUnbound]

Thanks for looking over this @sarayourfriend! I wanted to at least try it to see the breadth of the changes, but similarly I'm frustrated by how the linting adds so much seemingly useless text 😓 I think of the changes I made, the ones I actually enjoyed the products of were:

• Imperative first sentence (e.g. Return <x> rather than Returns <x> or This is a function that returns <x>) [examples from this changeset: D401]
• Whitespace/newline standardization for docstrings (e.g. some docstrings could be a single line, first text after a docstring should be on a newline, newline after a short summary, etc) [examples from this changeset: D205, D200, D209]
• Making sure we don't accidentally use quadruple quotes (there were a few instances of this!) [D300]

Rules I could totally do away with:

• Requiring a period at the end of the first line (as you pointed out) [D400]
• Meaningless summaries (particularly for modules) [I think this might have been D400 as well?]

All that to say, it sounds like we may want to also ignore D400 and possibly D205 (the "1 blank line after summary line & description") if we want to get rid of the things we feel cause the most noise, while still keeping the other whitespace, quote, and content checks. What do you think @WordPress/openverse-catalog?

[dhruvkb]

I made a similar set of changes on the API and for the most part, I feel like a lot of the changes were good and positive for the documentation. It was a bit of hard work to fix all of its complaints but it was a one-time effort. I would recommend keeping this lint check.

[AetherUnbound]

Per discussion in our Make WP chat, we're going to keep the check in general but remove the two rules shared above in the upstream repo. I'll take care of that this week 🙂

[AetherUnbound]

Okay, rebased with the necessary changes! I kept most of the changes I had made, except for the . at the end of the first line on all our DAG docstrings and a few other annoying nuisances we pointed out above.