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

Data access api README/Docs update #744

Open
wants to merge 44 commits into
base: dev
Choose a base branch
from

Conversation

anamanica
Copy link
Contributor

@anamanica anamanica commented Aug 7, 2024

Change Summary

Overview

This PR combines the information in the README for the data access api into the docs.

(I will have to cut down the README in a PR on the imap-data-access repo.)

New Dependencies

None

New Files

None

Deleted Files

None

Updated Files

  • index.rst
    • adding in README information

Testing

None

@anamanica anamanica marked this pull request as ready for review August 7, 2024 21:28
@bourque bourque requested a review from greglucas August 8, 2024 15:30
@bourque
Copy link
Collaborator

bourque commented Aug 8, 2024

Adding @greglucas as a reviewer here since he wrote most of this documentation and may have opinions about this!

@bourque
Copy link
Collaborator

bourque commented Aug 8, 2024

@greglucas
Copy link
Collaborator

@greglucas Totally agree, and I think the end goal is to have all of the documentation pertaining to the Data Access API in this RtD page, and then basically have the imap-data-access repo README just say something like "For documentation how to use this, go to the official docs here". Does that sound good to you?

I would personally prefer the opposite direction 🤷 I think a package should be standalone if possible and the README should describe whats going on within that package without needing to click somewhere else. I just don't know how feasible any of it is to go either direction.

Merge branch 'dev' of github.com:IMAP-Science-Operations-Center/imap_processing into data-access-api-README
Merge branch 'dev' of github.com:IMAP-Science-Operations-Center/imap_processing into data-access-api-README
@anamanica anamanica requested a review from bourque August 14, 2024 19:27
Merge branch 'dev' of github.com:IMAP-Science-Operations-Center/imap_processing into data-access-api-README
Comment on lines 19 to 22
Command Line Utility
====================
To Install
----------
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Command Line Utility
====================
To Install
----------
Command Line Utility
--------------------
To Install
^^^^^^^^^^

It looks like these headings (and throughout this document) are still messing up the main RtD page layout. I think these need to be changed to another level 'down' in markdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Sorry, Okay. I think I understand what you are saying now! Should I do this with every heading?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, except for the very first one (line 4), which is fine as is.

Comment on lines 44 to 46
.. openapi:: openapi.yml
:group:
:include: /query
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part is describing the REST API specification, so it should be moved to under that section.

Comment on lines 57 to 59
.. openapi:: openapi.yml
:group:
:include: /download
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to the last comment, this is describing the REST API so should also be moved (same with the /upload section.

@anamanica anamanica requested a review from bourque August 16, 2024 17:27
@greglucas greglucas added the Repo: Documentation Improvements or additions to documentation label Aug 20, 2024
Copy link
Collaborator

@bourque bourque left a comment

Choose a reason for hiding this comment

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

Thanks for these updates, this is looking really good! As Greg suggested, I think it is worth exploring if we can automatically include the contents of the imap-data-access README file in here, but that can be explored in a separate PR if you'd like.

I just have a few suggestions to help clarify some areas or make them a bit more readable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this document is quite lengthy now and have several sections and subsections, I suggest to add a table of contents somewhere towards the top. (Could be done in a future PR if desired)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevermind, Ana pointed out to me that there is a built-in table of contents on ReadTheDocs, which I totally missed!

docs/source/data-access-api/index.rst Outdated Show resolved Hide resolved
docs/source/data-access-api/index.rst Outdated Show resolved Hide resolved
docs/source/data-access-api/index.rst Outdated Show resolved Hide resolved
docs/source/data-access-api/index.rst Outdated Show resolved Hide resolved
docs/source/data-access-api/index.rst Outdated Show resolved Hide resolved
docs/source/data-access-api/index.rst Outdated Show resolved Hide resolved
docs/source/data-access-api/index.rst Outdated Show resolved Hide resolved
docs/source/data-access-api/index.rst Outdated Show resolved Hide resolved
docs/source/data-access-api/index.rst Outdated Show resolved Hide resolved
@bourque bourque self-requested a review August 30, 2024 02:10
Copy link
Collaborator

@bourque bourque left a comment

Choose a reason for hiding this comment

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

Thanks for making those updates, this looks good to go!

@bourque
Copy link
Collaborator

bourque commented Sep 16, 2024

@anamanica Just a heads up that you will likely run into merge conflicts once #844 is merged, since that PR is changing the directory names a bit.

@maxinelasp
Copy link
Contributor

@anamanica are you planning on merging this soon?

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

Successfully merging this pull request may close these issues.

4 participants