Skip to content

Improve WaterSSTP helper class documentation.#809

Closed
24sharkS wants to merge 6 commits into
Cantera:masterfrom
24sharkS:master
Closed

Improve WaterSSTP helper class documentation.#809
24sharkS wants to merge 6 commits into
Cantera:masterfrom
24sharkS:master

Conversation

@24sharkS
Copy link
Copy Markdown

@24sharkS 24sharkS commented Feb 16, 2020

Checklist

  • There is a clear use-case for this code change
  • The commit message has a short title & references relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • The pull request is ready for review

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

Fixes #646

Changes proposed in this pull request

  • Added non-instantiability property in WaterSSTP helper class documentation.

@24sharkS 24sharkS requested a review from bryanwweber February 16, 2020 17:18
@bryanwweber
Copy link
Copy Markdown
Member

@24sharkS Thank you for the contribution! I'm going to ask @decaluwe to take a look, since he is the creator of that issue.

@decaluwe
Copy link
Copy Markdown
Member

Hi, @24sharkS, many thanks! I think this is definitely a good start, and may end up being sufficient.

I think it probably answers the main question that I posed in #646. The only remaining question on my mind is whether any additional info could give the interested user a better sense of how these files relate to one another. For example, are each of these called directly by WaterSSTP methods? Or do some of these house sub-routines that are called by one of the other helper classes?

Reading over the documentation (both your improved documentation here, plus the original documentation in WaterSSTP.h, I do not really get an understanding of just how this class works. It is possible I am just missing it, but I think that should be the overriding goal of this documentation. If you could add a little bit to help explain how and when these files are called, and possibly some text to WaterSSTP.h to help explain in general how the class operates, I think this would be a great improvement.

Thanks again,
Steven

@24sharkS
Copy link
Copy Markdown
Author

@decaluwe yes I would be happy to do so.
Thanks for the review !!

Copy link
Copy Markdown
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Rather than updating the docstrings that apply to the header and implementation files (i.e. the first Doxygen comment block occuring in each file), I would suggest that any additional documentation be added to the docstrings for the classes (occurring just before the declaration of the class). I think the documentation pages for the classes (e.g. WaterPropsIAPWS) are generally far more useful than those for the files (e.g. WaterPropsIAPWS.h).

@bryanwweber
Copy link
Copy Markdown
Member

@24sharkS Are you planning to update this pull request? If not, it's fine, I'm just trying to do a bit of cleanup. Thank you!

@24sharkS
Copy link
Copy Markdown
Author

@bryanwweber Sorry, I wasn't able to update it. If you wan't to close it, it's totally fine.

@bryanwweber
Copy link
Copy Markdown
Member

Thanks @24sharkS !

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.

Improve WaterSSTP helper class documentation

4 participants