Skip to content

Conversation

@spencerkclark
Copy link
Member

🚀 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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2021

Codecov Report

Merging #87 (c372a4d) into main (837fec3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
nc_time_axis/__init__.py 94.30% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 837fec3...c372a4d. Read the comment docs.

@spencerkclark spencerkclark mentioned this pull request Aug 17, 2021
@spencerkclark
Copy link
Member Author

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!

@spencerkclark spencerkclark marked this pull request as ready for review August 18, 2021 12:49
Copy link
Contributor

@aulemahal aulemahal left a 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!

@trexfeathers
Copy link
Contributor

Amazing work @spencerkclark! I won't pretend to have looked over the code, but the render looks beautiful 🎁

@trexfeathers
Copy link
Contributor

... and I should take @aulemahal's word that this is acceptable and merge without delay!

@trexfeathers trexfeathers merged commit d65f545 into SciTools:main Aug 18, 2021
@spencerkclark spencerkclark deleted the documentation branch August 18, 2021 17:49
@spencerkclark
Copy link
Member Author

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.

Copy link
Member

@pelson pelson left a 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
Copy link
Member

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)

Copy link
Member Author

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)
Copy link
Member

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 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Great thanks! I added a short sentence to this effect in the top-level summary of the initial release in #89, which hopefully makes these contributions by both you and @lbdreyer a little more prominent.

: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
Copy link
Member

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

Copy link
Member Author

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.

@spencerkclark
Copy link
Member Author

I appreciate your comments @pelson! See #89 for a followup.

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.

Documentation

5 participants