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

DOC: initial documentation #50

Merged
merged 2 commits into from
Apr 21, 2023
Merged

DOC: initial documentation #50

merged 2 commits into from
Apr 21, 2023

Conversation

ntessore
Copy link
Contributor

Description

This pull request is to create the initial documentation skeleton for the Cosmology API.

PR Checklist

  • Give a detailed description of the PR above.
  • Document changes in the CHANGES.rst file. See existing changelog for examples.
  • Add tests, if applicable, to ensure code coverage never decreases.
  • Make sure the docs are up to date, if applicable, particularly the docstrings and RST files in docs folder.
  • Ensure linear history by rebasing, when requested by the maintainer.

@ntessore ntessore added the documentation Improvements or additions to documentation label Apr 13, 2023
@ntessore ntessore added this to the v0.1 milestone Apr 13, 2023
@ntessore ntessore requested a review from nstarman April 13, 2023 23:40
@ntessore
Copy link
Contributor Author

Docs preview here. The API sections for components and attributes will be properly filled after #48.

@ntessore
Copy link
Contributor Author

ntessore commented Apr 13, 2023

@nstarman I have opted here to use the summary-style tables for properties and methods in the protocols, which is much more compact than the full form.

I also decided against creating individual pages for each method, because they contain almost no added value. We should explain the deal with input and output array types in prose in the "how to use" section for users. Other than that, I would argue that no method has a signature that warrants more than the one line summary shown in the table.

Should any set of functions require proper explanation (e.g. the meaning of all distances), I believe those should go on prose pages with a proper explanation (e.g. "Distance functions"), and not the API docs. These should always remain short and with only the necessary information.

@ntessore ntessore force-pushed the nt/docs branch 2 times, most recently from 45a3926 to b8b111d Compare April 14, 2023 00:06
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2023

Codecov Report

Merging #50 (0b6c4ec) into main (cdea954) will decrease coverage by 0.25%.
The diff coverage is 75.00%.

❗ Current head 0b6c4ec differs from pull request most recent head 1175fc4. Consider uploading reports for the commit 1175fc4 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
- Coverage   82.49%   82.24%   -0.25%     
==========================================
  Files          13       13              
  Lines         377      383       +6     
==========================================
+ Hits          311      315       +4     
- Misses         66       68       +2     
Impacted Files Coverage Δ
src/cosmology/api/_constants.py 81.81% <ø> (ø)
src/cosmology/api/_core.py 78.94% <ø> (ø)
src/cosmology/api/_namespace.py 80.00% <ø> (ø)
src/cosmology/api/_distances.py 82.85% <75.00%> (-1.52%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ntessore
Copy link
Contributor Author

@nstarman I managed to hack autosummary to generate something close to what I originally had in mind: a flat list of all methods and properties, and the protocol hierarchy, all linking to the same documentation page with a single source of truth.

The autosummary hack is horrible, but I don't see another way.

@nstarman
Copy link
Contributor

The docs look great! In particular, getting the "On this page" to not include the prefix is a major improvement. I'll do a more detailed review and contribute some more docs, probably in the form of docstrings, but a few notes for now:

@nstarman
Copy link
Contributor

nstarman commented Apr 20, 2023

Failing because #53 is not merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this in the test suite. It s good to track which libraries support float addition, for the discussion of the input type.

@nstarman
Copy link
Contributor

Oh no. Rebasing muddled the commit history.

@ntessore ntessore force-pushed the nt/docs branch 3 times, most recently from d1a424e to 2760095 Compare April 21, 2023 09:21
@ntessore
Copy link
Contributor Author

@nstarman Ok, I think that this PR is ready on the technical side, it only needs some more content. Do you want to merge first or do everything in this PR?

@ntessore ntessore force-pushed the nt/docs branch 2 times, most recently from e1913fd to 0b6c4ec Compare April 21, 2023 11:18
@ntessore ntessore marked this pull request as ready for review April 21, 2023 16:52
@nstarman
Copy link
Contributor

Let's merge first! We'll get the docs as solid as possible in quick followups.

Copy link
Contributor

@nstarman nstarman left a comment

Choose a reason for hiding this comment

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

Docs look great!

if parent is cosmo:
_, _, membername = name.rpartition(".")
protocols = [
f"{cls.__qualname__}.{membername}"
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this could be made a back-link to the parent Protocol?

Copy link
Contributor

Choose a reason for hiding this comment

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

For a followup PR.

@nstarman nstarman merged commit ae5c709 into main Apr 21, 2023
@nstarman nstarman deleted the nt/docs branch April 21, 2023 18:15
@nstarman nstarman mentioned this pull request Apr 21, 2023
Copy link

@89535399501 89535399501 left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs documentation Improvements or additions to documentation infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants