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

Documentation Cleanup: First Pass #23

Merged
merged 5 commits into from
Jun 3, 2021
Merged

Documentation Cleanup: First Pass #23

merged 5 commits into from
Jun 3, 2021

Conversation

akaszynski
Copy link
Contributor

Documentation Cleanup

This is the first pass at genuine documentation cleanup. There are a variety of issues and I hope this PR can help to establish the expected documentation coding style, module docstrings, etc.

Biggest feedback from initial users has been doc standards and examples. Let's get this improved in time for initial release.

Module Docstrings

We're following the numpydoc standards, so it's necessary to include "Sections" to get the documentation formatted right. This is primarily to sphinx to be able to build the docs. For additional details, see: Documenting Modules

What does this mean for this repository? We need to go from this:
https://github.com/pyansys/PyAEDT/blob/9711f5b565e2bcf6517fae0acf64f5a7406a82ce/pyaedt/desktop.py#L1-L43

to this:
https://github.com/pyansys/PyAEDT/blob/15037421203152c10d3e3cd5b602011d04f8f5ac/pyaedt/desktop.py#L1-L25

The changes:

  • Using valid numpydoc sections. At the minimum we need a one line description (summary), optionally followed by an extended summary. For important modules, we probably should include this. Module docstrings should have examples, but no parameters. Parameters should be specified within class, method, and function docstrings.
  • Examples are in a pythonic format using >>>. Users expect that from numpy style docs, and sphinx renders this correctly. Text that is not after >>> will be rendered as plain text, and text after >>> or ... (indication continuation), will be rendered as python code by Sphinx.
  • Complete removal of copyright boilerplate. Legal has approved this provided our LICENCE.txt meets their standards. We need one small change there, but that's for another PR.

Class Docstrings

These need to be cleaned up. For example, in the following class docstring there's ** directives, which should not be used within class summaries (or in the docstrings in general). Parameters have not been specified, the Return section should not be included for classes, and we're missing the Examples section.
https://github.com/pyansys/PyAEDT/blob/9711f5b565e2bcf6517fae0acf64f5a7406a82ce/pyaedt/application/Analysis.py#L44-L57

Method Docstrings

These need to follow all the aforementioned directives for module and class docstrings, but in addition, properties and setters need special treatment. For example:
https://github.com/pyansys/PyAEDT/blob/9711f5b565e2bcf6517fae0acf64f5a7406a82ce/pyaedt/application/Analysis.py#L127-L140

Setters do not get docstrings at all, and the Parameters section does not need to be specified. The sphinx documentation doesn't generate it. Properties do not need to be spelled out as properties. Cleaned up docstring would look like:

    @property
    def CoordinateSystemAxis(self):
        """CoordinateSystemAxis Constant
        
        Coordinate System Axis Constants Tuple (.X, .Y, .Z)

        Returns
        -------
        tuple
            Coordinate System Axis Constants Tuple (.X, .Y, .Z)

        Examples
        --------
        >>> hfss.analysis.CoordinateSystemAxis
        (1, 0, 0)
        """
        return CoordinateSystemAxis()

Note that I have no idea what the actual return values of this should be, to the example is probably wrong.

Where we go from here

@maxcapodi78, I'd like you to involve ACE to work on the examples and at the same time develop unit tests so we can improve coverage. I've found that you can often copy a unit test to an example and vice versa.

@PipKat, could you please take a look at this PR and attempt to fix the module docstrings that I didn't cover by following the pattern presented here? It's less automatic than I had hoped since some modules are have examples that need to be reformatted.

@akaszynski
Copy link
Contributor Author

akaszynski commented Jun 3, 2021

Also, style check (just spelling right now) is expected to fail. Once this is merged, would be nice if we could go through and cleanup the spelling errors in another PR.

@akaszynski akaszynski merged commit ab4cac2 into main Jun 3, 2021
@PipKat
Copy link
Member

PipKat commented Jun 3, 2021

@akaszynski I can use your PR as a model for fix the module docstrings and can begin doing so tomorrow. I do have a process question, however. Do I make changes to the files in your same branch or create a new branch. The PR message saying that the pull request was successfully merged and closed makes me believe I do so in a new branch--but I wasn't certain.

@akaszynski
Copy link
Contributor Author

The PR message saying that the pull request was successfully merged and closed makes me believe I do so in a new branch--but I wasn't certain.

You'll want to create a new PR for this. Generally the approach is to merge and delete the branch. I kept the branch around so you could look at it for reference.

BTW, expectation is for @PipKat to work on just the module docstrings at this point to avoid merge conflicts by modifying classes. I'd probably do two or three PRs to be able to have us review in smaller chunks to avoid any potential conflicts that way as well.

@akaszynski akaszynski deleted the docs/desc_cleanup branch June 8, 2021 04:52
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.

2 participants