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

Rule D417 #3072

Merged
merged 8 commits into from
Jul 3, 2024
Merged

Rule D417 #3072

merged 8 commits into from
Jul 3, 2024

Conversation

jamshale
Copy link
Contributor

@jamshale jamshale commented Jun 28, 2024

Updates function documentation to the appropriate level needed for rule D417 https://docs.astral.sh/ruff/rules/undocumented-param/. Helps developers understand and more quickly use functions if these are accurate.

Mostly used co-pilot to fill in wrong or missing arguments. Reviewed most of it and it looked like it was doing a good job.

@jamshale jamshale force-pushed the rule-D417 branch 3 times, most recently from e24c269 to 81b9581 Compare June 28, 2024 20:32
@jamshale jamshale marked this pull request as ready for review June 28, 2024 20:42
@swcurran
Copy link
Contributor

Way cool — that’s wild. And the config change means these will be kept up to date?

@jamshale — would you mind look at the warnings we are getting when we generate the read the docs? To see them:

  • go into the docs folder
  • Run the two instructions in the UpdateRTD.md file
    • The sphinx command to copy the latest set of files (e.g. rm -rf generated; sphinx-apidoc….)
    • The sphinx-build command

No worries if it is not obvious what is needed. Obvs, you need to install sphinx (or use docker) to run the commands.

FYI — I’m using version sphinx-build 5.3.0.

@jamshale
Copy link
Contributor Author

jamshale commented Jul 2, 2024

Yes. It will at least fail the ruff lint check that is done with the unit tests and force developers to keep the function descriptions up to date. I think we should fully switch to ruff for linting, because right now ruff and black are both tools that can do the same thing. Ruff is just more popular now and faster. Not important to switch immediately, though.

I'll have a look at the doc warnings 👀

@dbluhm
Copy link
Contributor

dbluhm commented Jul 2, 2024

Full agreement from me on switching to ruff for formatting too; it is noticeably faster than black

@jamshale
Copy link
Contributor Author

jamshale commented Jul 2, 2024

@swcurran Did you used to get any warnings or errors when running sphinx-build? I fixed one set of warnings related to these changes but am getting other warnings for unrelated documentation.

I can continue looking into them but might create another task for it. I'm having quite a bit of trouble trying to resolve them effectively.

@jamshale
Copy link
Contributor Author

jamshale commented Jul 3, 2024

I'm slowly figuring out how to fix the sphinx errors and warnings. I'll include the changes in this PR once I resolve them all.

@swcurran
Copy link
Contributor

swcurran commented Jul 3, 2024

Should you push that to another PR? Don’t want to take too much of your time on it.

@jamshale
Copy link
Contributor Author

jamshale commented Jul 3, 2024

We can merge this the way it is. They warnings and errors only effect the individual docs. The rest will still be generated.

I think I should be done soon though. Fixed most of them.

@jamshale
Copy link
Contributor Author

jamshale commented Jul 3, 2024

This is updated and should be good to go now.

@jamshale jamshale requested review from ianco and dbluhm July 3, 2024 20:14
Copy link

sonarqubecloud bot commented Jul 3, 2024

@swcurran swcurran merged commit 9924096 into openwallet-foundation:main Jul 3, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants