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

edited data dictionary description #3781

Merged
merged 12 commits into from
Sep 5, 2024

Conversation

Nancy9ice
Copy link
Contributor

Overview

Closes this Google Season of Docs Task:
There should be a description of what the Data dictionaries sub-page is about before the list

What problem does this address?
This adds descriptions for the Data dictionary sub-headings in the readthedocs to aid clarity for users.

Testing

How did you make sure this worked? How can a reviewer verify this?

I ran the command sphinx-build -b html docs docs/_build/html and checked if my changes reflected correctly locally

@aesharpe @zaneselvans

@zaneselvans
Copy link
Member

For testing the documentation output, I think it will usually be preferable to run:

make docs-build

Which rebuilds the documentation from scratch, after removing all dynamically generated outputs, which can occasionally be leftover after a failed docs build.

While running the Sphinx command directly can be faster if there's a single file you're editing and you want to see the results of your edits. The CI on GitHub will attempt to rebuild that docs from scratch with the make command though.

@Nancy9ice
Copy link
Contributor Author

Okay, noted. I'll apply this to the other pull requests I'll work on later

@e-belfer e-belfer added the docs Documentation for users and contributors. label Aug 29, 2024
@e-belfer e-belfer self-requested a review August 29, 2024 20:19
Copy link
Member

@e-belfer e-belfer left a comment

Choose a reason for hiding this comment

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

Looks good! I just have a few minor wording suggestions.

e-belfer
e-belfer previously approved these changes Aug 30, 2024
Copy link
Member

@e-belfer e-belfer left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@e-belfer e-belfer self-requested a review August 30, 2024 13:47
@e-belfer e-belfer dismissed their stale review August 30, 2024 13:47

Missed the docs failure!

@e-belfer
Copy link
Member

@Nancy9ice Looks like there are two "line too long" errors in lines 8 and 20 of index.rst. Once they're split, should be good to go!

docs/data_dictionaries/index.rst:8: D001 Line too long
docs/data_dictionaries/index.rst:20: D001 Line too long

@Nancy9ice
Copy link
Contributor Author

@Nancy9ice Looks like there are two "line too long" errors in lines 8 and 20 of index.rst. Once they're split, should be good to go!

docs/data_dictionaries/index.rst:8: D001 Line too long
docs/data_dictionaries/index.rst:20: D001 Line too long

Resolved!

@zaneselvans
Copy link
Member

Hmm, docs build still seems to be failing. Did you push?

@aesharpe
Copy link
Member

aesharpe commented Sep 2, 2024

Maybe out of scope for this PR, but @Nancy9ice I also think it would be a good idea to change the name of this data dictionary to "Raw FERC Form 1 Data Dictionary" vs. "FERC For 1 Data Dictionary" if it's not too hard.

@Nancy9ice
Copy link
Contributor Author

Hmm, docs build still seems to be failing. Did you push?

Sorry, I just noticed I didn't push. I've pushed it now. Please confirm that the make docs-build test now runs successfully

@Nancy9ice
Copy link
Contributor Author

Maybe out of scope for this PR, but @Nancy9ice I also think it would be a good idea to change the name of this data dictionary to "Raw FERC Form 1 Data Dictionary" vs. "FERC For 1 Data Dictionary" if it's not too hard.

I just added this and pushed it. Thanks

Copy link
Member

@aesharpe aesharpe left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks @Nancy9ice

@aesharpe aesharpe added this pull request to the merge queue Sep 5, 2024
@aesharpe aesharpe removed this pull request from the merge queue due to a manual request Sep 5, 2024
@aesharpe aesharpe added this pull request to the merge queue Sep 5, 2024
Merged via the queue into catalyst-cooperative:main with commit 3bce7e5 Sep 5, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation for users and contributors.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants