Skip to content

Conversation

@JasonGrace2282
Copy link
Member

@JasonGrace2282 JasonGrace2282 commented Jul 22, 2024

Replace flake8-docstrings with pydocstyle. This should allow for fixing/standardization of doc strings automatically by ruff (via pre-commit).
If you disagree with any of the changes, feel free to comment!

Note

We might want to add this to git blame ignore sha list

Documentation

https://manimce--3881.org.readthedocs.build/en/3881/

@JasonGrace2282 JasonGrace2282 requested a review from Viicos July 22, 2024 17:54
Copy link
Member

@Viicos Viicos left a comment

Choose a reason for hiding this comment

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

I like it, I generally prefer having single line docstrings not on 3 lines, so looks good!

@Viicos
Copy link
Member

Viicos commented Jul 23, 2024

Does this need any change in the Ruff pyproject config?

@JasonGrace2282
Copy link
Member Author

JasonGrace2282 commented Jul 23, 2024

Does this need any change in the Ruff pyproject config?

If you're talking about https://docs.astral.sh/ruff/rules/fits-on-one-line/ then no, it's included with pydocstyle and not ignored.
Other than that the only change I made to the pyproject was adding pydocstyle and ignoring some rules.

@JasonGrace2282
Copy link
Member Author

JasonGrace2282 commented Jul 23, 2024

This will probably need to be reviewed again once I push a commit fixing the docs build, marking as a draft for now.

I was also hoping this PR would fix the SyntaxWarnings, but I still can't seem to find where they originate 🤔

@JasonGrace2282 JasonGrace2282 marked this pull request as draft July 23, 2024 14:05
@JasonGrace2282 JasonGrace2282 marked this pull request as ready for review July 23, 2024 14:24
@JasonGrace2282 JasonGrace2282 added the enhancement Additions and improvements in general label Jul 24, 2024
@JasonGrace2282 JasonGrace2282 requested a review from Viicos July 24, 2024 16:28
@Viicos
Copy link
Member

Viicos commented Jul 25, 2024

If you're talking about https://docs.astral.sh/ruff/rules/fits-on-one-line/ then no, it's included with pydocstyle and not ignored.
Other than that the only change I made to the pyproject was adding pydocstyle and ignoring some rules.

I was talking about the ruff pyproject configuration we have, but it seems it was added in your last commit 👍

I was also hoping this PR would fix the SyntaxWarnings, but I still can't seem to find where they originate 🤔

Do you have an example of such a SyntaxWarning being raised?

@JasonGrace2282
Copy link
Member Author

Do you have an example of such a SyntaxWarning being raised?

After running make cleanall and then make html I end up with messages like this

<string>:28: SyntaxWarning: invalid escape sequence '\p'
<string>:14: SyntaxWarning: invalid escape sequence '\D'
<string>:14: SyntaxWarning: invalid escape sequence '\c'

Interestingly, the build log for the docs CI doesn't show it, so it might be some sort of weird setup on my part.

@MrDiver
Copy link
Collaborator

MrDiver commented Jul 25, 2024

Does this automagically work with the CI ? because we should probably make sure it's done automagically here too

@JasonGrace2282
Copy link
Member Author

Does this automagically work with the CI ? because we should probably make sure it's done automagically here too

A lot of the formatting ones do (see the ones marked with tools here https://docs.astral.sh/ruff/rules/#pydocstyle-d)

Sometimes, with regards to \s manual changes are needed (see my manual changes commit), but it should be relatively uncommon.

@JasonGrace2282
Copy link
Member Author

JasonGrace2282 commented Jul 26, 2024

This PR can now also remove flake8 from Manim!

@JasonGrace2282
Copy link
Member Author

JasonGrace2282 commented Aug 30, 2024

If there are no other thoughts on this, I might merge it within the next few days.
(If anyone wants to review it, but needs more time, feel free to let me know and I'll wait).

@Viicos
Copy link
Member

Viicos commented Aug 31, 2024

After running make cleanall and then make html I end up with messages like this

<string>:28: SyntaxWarning: invalid escape sequence '\p'
<string>:14: SyntaxWarning: invalid escape sequence '\D'
<string>:14: SyntaxWarning: invalid escape sequence '\c'

Is this still an issue now? Python 3.12 started warning about invalid escape sequences but iirc it is planned to error in some later versions. You can use the r prefix if necessary. Other than that, happy to see this one merged!

@JasonGrace2282
Copy link
Member Author

JasonGrace2282 commented Sep 1, 2024

After running make cleanall and then make html I end up with messages like this

<string>:28: SyntaxWarning: invalid escape sequence '\p'
<string>:14: SyntaxWarning: invalid escape sequence '\D'
<string>:14: SyntaxWarning: invalid escape sequence '\c'

Is this still an issue now? Python 3.12 started warning about invalid escape sequences but iirc it is planned to error in some later versions. You can use the r prefix if necessary. Other than that, happy to see this one merged!

Ah right I'd forgotten about that change in python 3.12, now it makes sense why it wasn't in the ci log (which runs 3.11).

I think I figured out why the SyntaxWarning appears - it's from when the Manim directive is rendering the docs examples, not at the Sphinx level. As such, they're unrelated to the PR, and considering the size of the PR already, I think I'll tackle fixing them in a follow-up PR.

@JasonGrace2282 JasonGrace2282 merged commit 1097ed9 into ManimCommunity:main Sep 1, 2024
@JasonGrace2282 JasonGrace2282 deleted the ruff-pydoc branch September 1, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Additions and improvements in general

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants