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

Reorganize contributing docs + add process description. #3044

Merged
merged 3 commits into from
Dec 6, 2023

Conversation

jdangerx
Copy link
Member

Reorganize & streamline contributing docs - hopefully makes it easier for someone who wants to help out to figure out what specifically they can do.

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (77c8e79) 92.6% compared to head (4b94ff5) 92.6%.
Report is 8 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##             dev   #3044     +/-   ##
=======================================
- Coverage   92.6%   92.6%   -0.0%     
=======================================
  Files        134     134             
  Lines      12535   12535             
=======================================
- Hits       11607   11606      -1     
- Misses       928     929      +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@e-belfer e-belfer self-requested a review November 20, 2023 20:32
@jdangerx jdangerx force-pushed the contributing-docs-streamline branch 2 times, most recently from 782a054 to a431dac Compare November 28, 2023 19:04
Copy link
Member Author

Choose a reason for hiding this comment

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

🤷 seemed like maybe something useful to have? But also we can scrap it for now and new dataset requests can come in through "Feature Request."

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to keep this for now! Simple enough. Might change later.

.github/pull_request_template.md Show resolved Hide resolved
- [ ] All CI checks are passing. [Run tests locally to debug failures](https://catalystcoop-pudl.readthedocs.io/en/latest/dev/testing.html#running-tests-with-tox)
- [ ] Make sure you've included good docstrings.
```[tasklist]
# Remaining work
Copy link
Member Author

Choose a reason for hiding this comment

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

A lot of times people open PRs and then have some sort of TODO list to keep track of "what else needs to change before this is ready for prime time." Seems like the PR checklist should just combine with that, but I'm not quite sure how to communicate that clearly.

Separately, I deleted a bunch of stuff that should get captured by the GH UI (keeping your branch up to date with base branch), CI checks, etc. Left local integration tests in place because that actually costs $ to run on CI willy-nilly.

@jdangerx jdangerx force-pushed the contributing-docs-streamline branch 2 times, most recently from 6890759 to 1cd95aa Compare November 30, 2023 16:46
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.

Maybe I'm more of a documentation maximalist, but I disagree about some of the deletions. Left a bunch of comments. Suspect having more eyes on this PR than just me would be helpful!

.github/ISSUE_TEMPLATE/new_dataset.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Show resolved Hide resolved
docs/CONTRIBUTING.rst Outdated Show resolved Hide resolved
docs/CONTRIBUTING.rst Show resolved Hide resolved
docs/CONTRIBUTING.rst Outdated Show resolved Hide resolved
.github/pull_request_template.md Show resolved Hide resolved
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, I just have a few non-blocking questions.

CONTRIBUTING.rst Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
@aesharpe aesharpe self-requested a review December 5, 2023 18:15
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.

Added some non-blocking comments!

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to keep this for now! Simple enough. Might change later.

- [ ] Defensive data quality/sanity checks in analyses & data processing functions.
- [ ] Update the [release notes](https://catalystcoop-pudl.readthedocs.io/en/latest/release_notes.html) and reference reference the PR and related issues.
- [ ] Do your own explanatory review of the PR to help the reviewer understand what's going on and identify issues preemptively.
- [ ] If updating analyses or data processing functions: write data quality checks
Copy link
Member

Choose a reason for hiding this comment

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

By data quality checks do you mean adding data validation tests or writing more defensive code? It might be nice to have a link to more detail here. I don't think we currently have any docs explaining data quality standards or examples of checks though.

CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated

.. IMPORTANT:: Already have a dataset in mind?

If you **need data that's not in PUDL** that we're missing in PUDL,
Copy link
Member

Choose a reason for hiding this comment

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

"Data that's not in PUDL that we're missing in PUDL sounds redundant.

CONTRIBUTING.rst Outdated Show resolved Hide resolved
Connect us with other organizations
-----------------------------------

For PUDL to make a bigger impact, we need to find more people who need the data.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should include our email somewhere here?

docs/CONTRIBUTING.rst Show resolved Hide resolved
@jdangerx jdangerx merged commit ce56db0 into dev Dec 6, 2023
14 of 15 checks passed
@jdangerx jdangerx deleted the contributing-docs-streamline branch December 6, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants