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

Update web-hosted API docs #87

Merged
merged 40 commits into from
Dec 18, 2021
Merged

Update web-hosted API docs #87

merged 40 commits into from
Dec 18, 2021

Conversation

yardasol
Copy link
Contributor

@yardasol yardasol commented Dec 10, 2021

Summary of changes

This PR updates the generated files for the web-hosted documentation to reflect the changes in #64.

Do not merge until changes from #90 have been applied.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Required for Merging

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
    • CI tests pass
    • Local tests pass (including Serpent2 integration tests)

Associated Issues and PRs

Associated Developers

Checklist for Reviewers

Reviewers should use this link to get to the
Review Checklist before they begin their review.

@yardasol yardasol linked an issue Dec 10, 2021 that may be closed by this pull request
@yardasol
Copy link
Contributor Author

yardasol commented Dec 13, 2021

Check for Sphinx metadata in the current version of the gh-pages branch to check if the formatting changes are due to a change in the Sphinx version

@yardasol
Copy link
Contributor Author

This PR is ready for review

Copy link
Contributor

@LukeSeifert LukeSeifert left a comment

Choose a reason for hiding this comment

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

Comments

depcode.DepcodeSerpent does not document parameters as being optional (i.e. codename = 'sss2' documented as codename (str) – Name of depletion code.) (possibly change to codename (str : optional)?).

In the "returns" sections, there is some inconsistency (i.e. get_nuc_name in depcode.DepcodeSerpent uses bullets and variable names while others list return type and describe what gets returned).
This is also present in app module (app.reprocessing and app.refill, seems to be different if multiple values are being returned).

Consider adding brief documentation for depcode.add_params.

In the process.Process and reactor.Reactor documentation, the class parameters are all listed in init, but in depcode they are listed directly under the class and not in init. (Though in separator.Separator, the parameters seem to be listed in both and in simulation.Simulation they are listed under init, so I'm not sure which way its meant to be).

Summary

Overall looks good! The things I pointed out are minor, so just let me know when you've made any changes you want to make and I'll merge.

@yardasol
Copy link
Contributor Author

Hi Luke, thanks for your comments. In the future could you use the in-line commenting system when making a review? It really helps speed up the processes of making changes based on review feedback.

depcode.DepcodeSerpent does not document parameters as being optional (i.e. codename = 'sss2' documented as codename (str) – Name of depletion code.) (possibly change to codename (str : optional)?).

Good point about specifying parameters with default values to be optional. As for the codename parameter, it isn't actually optional. I've removed it as a parameter in the documentation for the DepcodeSerpent class because there is no reason for the user to set this to anything other that 'serpent'.

Consider adding brief documentation for depcode.add_params.

I've removed the add_params helper function as I realized I can't use it in the way I want to.

In the "returns" sections, there is some inconsistency (i.e. get_nuc_name in depcode.DepcodeSerpent uses bullets and variable names while others list return type and describe what gets returned).
This is also present in app module (app.reprocessing and app.refill, seems to be different if multiple values are being returned).

Great catch! I totally missed this. Should be fixed now.

In the process.Process and reactor.Reactor documentation, the class parameters are all listed in init, but in depcode they are listed directly under the class and not in init. (Though in separator.Separator, the parameters seem to be listed in both and in simulation.Simulation they are listed under init, so I'm not sure which way its meant to be).

So this issue turns out to be a much bigger rabbit hole that I initially thought. There is a PEP 257 which tells us:

The docstring for a class should summarize its behavior and list the public methods and instance variables. If the class is intended to be subclassed, and has an additional interface for subclasses, this interface should be listed separately (in the docstring). The class constructor should be documented in the docstring for its init method. Individual methods should be documented by their own docstring.

I interpret this as telling us that we need to document our public

however, I checked some code examples in OpenMC they don't seem to follow this convention. There was also a lively discussion in this stack overflow post.

My opinion is that as long as it's consistent and makes sense, it shouldn't matter. Now from an implementation standpoint, I think we should follow PEP like @mehmeturkmen has done (good job Mehmet!). Additionally, most of the classes in the API seem to follow this. Will be fixing this as well.

@yardasol yardasol requested a review from LukeSeifert December 17, 2021 23:20
@LukeSeifert
Copy link
Contributor

Looks good! I'll make sure to use the in-line comments next time, I had forgotten to go back and put them in this time.

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.

[Bug]: Update the online API documentation
3 participants