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

Miscellaneous Python documentation fixes #1196

Merged
merged 14 commits into from
Feb 16, 2022

Conversation

speth
Copy link
Member

@speth speth commented Feb 14, 2022

Changes proposed in this pull request

  • Fix confusing labeling of all attributes of SolutionArray and Quantity class being labeled as property in the Sphinx docs
  • Make extended arguments (other than input file, phase name, and adjacent phases list) to Solution keyword-only (I'm not sure they would have worked as positional arguments in the first place)
  • Make extended arguments (other than reactants, products, and rate) to Reaction classes keyword-only (again, I think this was implicitly required in practice
  • Fix a bunch of formatting issues in the Sphinx documentation, mostly with how references to function arguments are formatted. Using double backticks gives normal monospace formatting.
  • Eliminate the remnants of the long-ago deprecated WallSurface class.

If applicable, fill in the issue number this pull request is fixing

Fixes #1078

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

Change the way passthrough functions to Quantity and SolutionArray
classes are handled so these functions don't get labeled as "property"
in the Sphinx docs and so their arguments will be shown.
This class represents both surfaces and edges
These types all share the same __cinit__ constructor, so the __init__
method also has to have a consistent signature, where the only
differences are in optional keyword-only arguments.
Classes that define an __init__ method don't need to have their
constructor signature defined in .rst file.

Add relevant arguments determined by by _SolutionBase.__cinit__ to
derived classes.
Using Sphinx 4.4.0 and Cython 0.29.26, none of these function
signatures generate warnings.
WallSurface was replaced by ReactorSurface in Cantera 2.3.
ischoegl
ischoegl previously approved these changes Feb 15, 2022
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

🎉 ... thanks for this fix @speth! While I'm not exactly sure what fixed the issue, I assume that it has to do with the signatures in the rst files? I verified with the GH artifact that things work. Also, thanks for making the formatting more consistent. Overall, this looks good to go from my perspective.

@speth
Copy link
Member Author

speth commented Feb 15, 2022

The trick to fixing the "property" labels was realizing that they actually were property objects, and this wasn't a weird Sphinx bug. These were just properties that returned the corresponding method of the underlying Solution object, but there's no way for Sphinx to know that. So the solution was instead to to wrap the methods that need to be forwarded without modification in a trivial function and copy the docstring from the parent function.

bryanwweber
bryanwweber previously approved these changes Feb 15, 2022
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Phew! Thanks for taking this on @speth. Detail orientation is my jam 😄 As such, I've left 44 comments for further improvements that I noted here! You're welcome 😝

I'll approve it though because they're all minor and you can merge this after updating them without me needing to re-approve.

interfaces/cython/cantera/composite.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/composite.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/composite.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/composite.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/composite.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/reactor.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/reactor.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/reactor.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/reactor.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/reactor.pyx Outdated Show resolved Hide resolved
speth and others added 3 commits February 15, 2022 19:52
Co-authored-by: Bryan W. Weber <[email protected]>
The exception to this is where the reference is specifically to the
package name, for instance when describing installation with pip or conda.
@bryanwweber bryanwweber merged commit 36717bd into Cantera:main Feb 16, 2022
@bryanwweber
Copy link
Member

Thanks @speth and @ischoegl! :shipit: In it goes!

@speth speth deleted the fix-1078-sphinx-properties branch July 23, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cantera documentation: Python member functions erroneously labeled as properties
3 participants