Skip to content

style: use numpy docstring and fix documentation rendering #125

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

Merged
merged 8 commits into from
Jul 28, 2025

Conversation

ycexiao
Copy link
Contributor

@ycexiao ycexiao commented Jul 26, 2025

What problem does this PR address?

Closes #118

What should the reviewer(s) do?

Please check the new docstring and rendered documentation.

Copy link
Contributor Author

@ycexiao ycexiao left a comment

Choose a reason for hiding this comment

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

@sbillinge, it's ready for review.

A scripts for converting those semi-format docstring into numpy style docstring is here https://github.com/ycexiao/scripts/blob/main/parse.py.

The original docstring is a little messy, so there might still be some places that need to be handled manually. Please check if the current docstring is acceptable, and then I can go on to make some small edits manually.

@@ -14,17 +14,12 @@ Subpackages
-----------

.. toctree::
diffpy.srfit.example_package

Submodules
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There seems to be no submodules in diffpy.srfit.

.. index:: release notes

.. mdinclude:: ../../../CHANGELOG.md
.. include:: ../../CHANGELOG.rst
Copy link
Contributor Author

Choose a reason for hiding this comment

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

diffpy.structure uses include and has a better title hierarchy

EquationFactory.
Attributes
----------
builders
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type is missing because it is not included in the original docstring.

@ycexiao ycexiao marked this pull request as ready for review July 26, 2025 20:22
@ycexiao
Copy link
Contributor Author

ycexiao commented Jul 26, 2025

Screenshots of the build documentation:

image image

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

Nicely done! there is just one tweak I think we should do, which is "Argument" -> "Parameters" per group standards.

All these docstrings are missing type, and default values for optional args, but that would be a lot of manual work so let's just do this bit by bit as we work on functions and classes. this is great for now. But please see if your script can do that parameters change?

constructor must accept the 'name' key word.
argkw -- Key word dictionary to pass to the argclass constructor
(default {}).
---------
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we call it "Parameters" and not "Arguments"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just took those names and didn't modify them. I will change this.

@sbillinge
Copy link
Contributor

Thanks for the screenshot. Could you share one or two where we can actually see the docstring though?

@ycexiao
Copy link
Contributor Author

ycexiao commented Jul 28, 2025

Nicely done! there is just one tweak I think we should do, which is "Argument" -> "Parameters" per group standards.

Yes. I can change this. It can be easily done.

But please see if your script can do that parameter change?

I can try to do it. Maybe a regular expression can help. But I think it might be a little difficult to cover all the cases. I will see what can be done to achieve the best cost-benefit efficiency.

@ycexiao
Copy link
Contributor Author

ycexiao commented Jul 28, 2025

Thanks for the screenshot. Could you share one or two where we can actually see the docstring though?

Screenshots with docstring
image

image

Copy link

codecov bot commented Jul 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.05%. Comparing base (6863ba0) to head (6e882b9).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #125   +/-   ##
=======================================
  Coverage   67.05%   67.05%           
=======================================
  Files          25       25           
  Lines        3160     3160           
=======================================
  Hits         2119     2119           
  Misses       1041     1041           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbillinge
Copy link
Contributor

@ycexiao for some reason this failed pre-commit in CI. Otherwise I would like to merge it. It is a beautriful PR! I know we want to fix the arguments -> Parameters but that could be in a different PR I guess? But let's fix the pre-commit issue before I merge.

If you hvae a fix for the arg -> parm almost ready to go let me know and we can do it on this PR, but otherwise let's put it off.

After this merges. Please could you run your scripts on the other diffpy projects that are affected?

@ycexiao
Copy link
Contributor Author

ycexiao commented Jul 28, 2025

@sbillinge, it's ready for review. The arg->param and pre-commit error is fixed. I will look at the same doc rendering error on other diffpy projects.

@sbillinge sbillinge merged commit b94a3a9 into diffpy:main Jul 28, 2025
3 checks passed
@ycexiao ycexiao mentioned this pull request Jul 29, 2025
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.

doc: fix documentation rendering
2 participants