-
Notifications
You must be signed in to change notification settings - Fork 35
Add documentation #87
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
Conversation
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## main #87 +/- ##
=======================================
Coverage 94.30% 94.30%
=======================================
Files 1 1
Lines 193 193
Branches 44 44
=======================================
Hits 182 182
Misses 6 6
Partials 5 5
Continue to review full report at Codecov.
|
…ime-axis into documentation
for more information, see https://pre-commit.ci
|
I think this should now be ready for review. I added pages for installation instructions, examples, and an API reference. I also tried to reconstruct release notes dating back to the first release, so that all contributors are acknowledged. See here for a preview of how things look. Hopefully this serves as a good base that we can iterate more on in future PRs! |
aulemahal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Clean and complete but concise documentation, well done! I believe we won't need much more in the future.
Did you add the whole release note page by hand?? That must have taken a long time!
|
Amazing work @spencerkclark! I won't pretend to have looked over the code, but the render looks beautiful 🎁 |
|
... and I should take @aulemahal's word that this is acceptable and merge without delay! |
|
Thanks @aulemahal and @trexfeathers! Indeed I wrote the release notes by hand -- it wasn't too bad -- I wanted to make sure everyone who laid the foundations of nc-time-axis before it had formal documentation was properly credited :). If anyone who knows the history better than I do has any corrections or additions I encourage them to make them. |
pelson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done 👍 👏
|
|
||
| For testing, nc-time-axis also depends on pytest: | ||
|
|
||
| * `pytest <https://docs.pytest.org/en/6.2.x>`_ >= 6.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems surprising to me that you would have the instructions on how to test something before you tell basic users how to install it. (I'm personally in the camp of not bothering to document installation requirements like this, and instead encode it in the setup.py (or equivalent), but that is not a universal position)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I went ahead and removed the testing requirements from the documentation in #89. It occurred to me there are additional requirements for building the documentation as well, which I had neglected document. I added those to the setup.cfg file where the testing requirements were documented. Testing and building documentation are both things that I would mainly expect developers to be concerned with, so it is reasonable that they would only be discoverable there.
It is true that having the minimum requirements documented in multiple places opens the door for them to get out of sync, but I do think it is helpful for non-developers to see those without having to look at the source code.
| <https://github.com/DPeterK>`_. | ||
|
|
||
|
|
||
| v1.0.0 (July 1st, 2016) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This history looks about right. As I recal, it was a prototype from me, and a productionisation from lbdreyer, so think you have the balance of credit about right 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| :py:class:`NetCDFTimeDateLocator`, :py:class:`NetCDFTimeDateFormatter`, and | ||
| :py:class:`NetCDFTimeConverter` (:pull:`2`). By `lbdryer | ||
| <https://github.com/lbdreyer>`_ and `Phil Elson <https://github.com/pelson>`_. | ||
| * Added unit and integration tests (:pull:`3`, :pull:`13`). By `lbdryer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo (throughout) of lbdryer -> lbdreyer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Also addressed in #89.
🚀 Pull Request
Closes #62
Description
This adds a basic start to the documentation using the sphinx RTD theme. Just posting this now to make sure I can get the web hooks configured properly. Will push more substantive updates later.