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.

Sign up for GitHub

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

Jump to bottom

DOC: add docstrings to units #172

Merged
merged 3 commits into from
Mar 30, 2023
Merged

DOC: add docstrings to units #172

merged 3 commits into from
Mar 30, 2023

Conversation

homosapien-lcy
Copy link
Contributor

As mentioned in issue #119 , it can help the users a great bit if we add doc string to the unit api. (Just realized there are so many different metric systems we have around the world...)

@corranwebster
Copy link
Contributor

corranwebster commented Mar 20, 2023 •
edited
Loading

Quick comment here - docstrings are usually fed into tools like Sphinx for producing auto-generated documentation (under the covers they use reStrcuturedText). You should make sure that the docstrings will produce good output with these tools. In particular, I think the way that you have indented your lists will not parse correctly with Sphinx.

@mdickinson mdickinson mentioned this pull request Mar 22, 2023
Closed
@homosapien-lcy
Copy link
Contributor Author

Hi Corran, Mark: Could you give me an example of how to structure the docstring of this type, I cannot find sth of similar type?

@homosapien-lcy
Copy link
Contributor Author

OK, I seem to figure it out from the top of other files... Have it updated

@mdickinson
Copy link
Member

The best thing to do here would be to run a Sphinx build with warnings turned on and look at (a) the warnings output, and (b) the generated docs.

If we don't already have autodoc-based mechanisms to build the docs, the the first step might be to put those in place.

@mdickinson
Copy link
Member

Looks like there is a python etstool.py docs command to build the documentation.

@mdickinson
Copy link
Member

It would also make sense to have a GitHub Actions workflow that runs the documentation build on each PR, so that we can detect regressions quickly.

@dpinte dpinte mentioned this pull request Mar 23, 2023
Open
@dpinte
Copy link
Member

dpinte commented Mar 23, 2023

Here is the output of the python etstools.py docs call. As you see there are some warning (related or not) to this PR. I can look at them and make sure they get fixed. That would be great!

$ python etstool.py docs
[EXECUTING] edm run -e scimath-test-3.6 -- sphinx-build -b html -d build/doctrees source build/html
Running Sphinx v3.5.4
making output directory... done
WARNING: favicon file 'et.ico' does not exist
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 12 source files that are out of date
updating environment: [new config] 12 added, 0 changed, 0 removed
reading sources... [100%] units/user_ref
/Users/dpinte/.edm/envs/scimath-test-3.6/lib/python3.6/site-packages/scimath-5.1.0.dev419-py3.6-macosx-10.9-x86_64.egg/scimath/units/convert.py:docstring of scimath.units.convert.convert:4: WARNING: Unexpected section title.

Parameters
----------
/Users/dpinte/.edm/envs/scimath-test-3.6/lib/python3.6/site-packages/scimath-5.1.0.dev419-py3.6-macosx-10.9-x86_64.egg/scimath/units/convert.py:docstring of scimath.units.convert.convert:13: WARNING: Unexpected section title.

Returns
-------
/Users/dpinte/.edm/envs/scimath-test-3.6/lib/python3.6/site-packages/scimath-5.1.0.dev419-py3.6-macosx-10.9-x86_64.egg/scimath/units/convert.py:docstring of scimath.units.convert.convert:18: WARNING: Unexpected section title.

Description
-----------
/Users/dpinte/.edm/envs/scimath-test-3.6/lib/python3.6/site-packages/scimath-5.1.0.dev419-py3.6-macosx-10.9-x86_64.egg/scimath/units/has_units.py:docstring of scimath.units.has_units.has_units:6: WARNING: Unexpected section title.

Parameters
----------
/Users/dpinte/.edm/envs/scimath-test-3.6/lib/python3.6/site-packages/scimath-5.1.0.dev419-py3.6-macosx-10.9-x86_64.egg/scimath/units/has_units.py:docstring of scimath.units.has_units.has_units:36: WARNING: Unexpected section title.

Description
-----------
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] units/user_ref
generating indices... genindex done
highlighting module code... [100%] scimath.units.unit_scalar
writing additional pages... search done
copying static files... done
copying extra files... done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded, 6 warnings.

The HTML pages are in build/html.

@homosapien-lcy
Copy link
Contributor Author

Here is the output of the python etstools.py docs call. As you see there are some warning (related or not) to this PR. I can look at them and make sure they get fixed. That would be great!

$ python etstool.py docs
[EXECUTING] edm run -e scimath-test-3.6 -- sphinx-build -b html -d build/doctrees source build/html
Running Sphinx v3.5.4
making output directory... done
WARNING: favicon file 'et.ico' does not exist
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 12 source files that are out of date
updating environment: [new config] 12 added, 0 changed, 0 removed
reading sources... [100%] units/user_ref
/Users/dpinte/.edm/envs/scimath-test-3.6/lib/python3.6/site-packages/scimath-5.1.0.dev419-py3.6-macosx-10.9-x86_64.egg/scimath/units/convert.py:docstring of scimath.units.convert.convert:4: WARNING: Unexpected section title.

Parameters
----------
/Users/dpinte/.edm/envs/scimath-test-3.6/lib/python3.6/site-packages/scimath-5.1.0.dev419-py3.6-macosx-10.9-x86_64.egg/scimath/units/convert.py:docstring of scimath.units.convert.convert:13: WARNING: Unexpected section title.

Returns
-------
/Users/dpinte/.edm/envs/scimath-test-3.6/lib/python3.6/site-packages/scimath-5.1.0.dev419-py3.6-macosx-10.9-x86_64.egg/scimath/units/convert.py:docstring of scimath.units.convert.convert:18: WARNING: Unexpected section title.

Description
-----------
/Users/dpinte/.edm/envs/scimath-test-3.6/lib/python3.6/site-packages/scimath-5.1.0.dev419-py3.6-macosx-10.9-x86_64.egg/scimath/units/has_units.py:docstring of scimath.units.has_units.has_units:6: WARNING: Unexpected section title.

Parameters
----------
/Users/dpinte/.edm/envs/scimath-test-3.6/lib/python3.6/site-packages/scimath-5.1.0.dev419-py3.6-macosx-10.9-x86_64.egg/scimath/units/has_units.py:docstring of scimath.units.has_units.has_units:36: WARNING: Unexpected section title.

Description
-----------
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] units/user_ref
generating indices... genindex done
highlighting module code... [100%] scimath.units.unit_scalar
writing additional pages... search done
copying static files... done
copying extra files... done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded, 6 warnings.

The HTML pages are in build/html.

Got it, just changed

dpinte
dpinte approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@dpinte dpinte left a comment

Choose a reason for hiding this comment

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

LGTM

@homosapien-lcy homosapien-lcy merged commit e06e318 into main Mar 30, 2023
@homosapien-lcy homosapien-lcy deleted the unit_doc_string branch March 30, 2023 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpinte dpinte dpinte approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@homosapien-lcy @corranwebster @mdickinson @dpinte