Page MenuHomeElementl

[Github Community] Add a PR template
ClosedPublic

Authored by yichendai on Dec 17 2020, 12:49 AM.
Tags
None
Referenced Files
F2982865: D5651.id29989.diff
Sun, May 28, 9:53 AM
F2982840: D5651.id30263.diff
Sun, May 28, 9:34 AM
Unknown Object (File)
Mon, May 15, 8:59 AM
Unknown Object (File)
Mon, May 8, 12:59 AM
Unknown Object (File)
Apr 19 2023, 8:40 AM
Unknown Object (File)
Apr 15 2023, 8:55 AM
Unknown Object (File)
Apr 12 2023, 7:54 PM
Unknown Object (File)
Apr 11 2023, 3:37 AM

Details

Summary

Add a PR template to reduce back and forth conversations between contributors and reviewers, making merging external pull requests life easier.

The Contributing.md will create a pop-up like below once a contributor create a new PR:

Screen Shot 2021-01-20 at 4.28.59 PM.png (610×2 px, 193 KB)

Reference: open-source-templates

Test Plan

The template can only be seen after landing on master. Fortunately, it doesn't affect anything else.

Diff Detail

Repository
R1 dagster
Branch
pr
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
yichendai retitled this revision from [RFC]Add a PR template to [Github Community] Add a PR template.Dec 17 2020, 1:00 AM

I think i would lean towards something more terse personally, maybe following our internal phabricator based set up of just having Summary and Test Plan then having some of these contextual pieces in those. Would be good to link to our directly to our contributing guide if possible.

Curious what @max thinks, or others on the team who have done more github contributions

Update: Add contributing guides

ill defer to @max to make the final call

.github/PULL_REQUEST_TEMPLATE.md
2–50

Just to give a sense of what I meant, this is an approximation of what I think would be a good place to start:

<!-- Hello Dagster contributor! It's great to have you with us! -->
<!-- Make sure to read https://docs.dagster.io/community/contributing  -->

## Summary
<!-- Describe your changes here, include things like: -->
<!-- related issue -->
<!-- type of change i.e. breaking change, new feature, or bug fix -->
<!-- motivation and context -->
<!-- screenshots -->


## Test Plan
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->

This is very much a matter of personal taste so its hard to say this is better or worse.

How does this template compare to other open source projects like dbt, gatsby, react, kubernetes?

.github/PULL_REQUEST_TEMPLATE.md
10

I don't feel like this should be a requirement since it adds yet another thing that contributors have to do (death by a thousand cuts)

18

Can we fold this into the "Describe your changes" section?

46

I think we should publish something about code style before asking users to check this. lint + code review should be sufficient till then imo

yichendai added inline comments.
.github/PULL_REQUEST_TEMPLATE.md
18

I agree that we fold these to one session like @alangenfeld suggested below.

46

The code style in contributing guide will pop up automatically when a contributor opens a PR once this diff is out.

.github/PULL_REQUEST_TEMPLATE.md
16

I think we could be more clear that we would like to see test coverage in the diff itself (as opposed to tests run in local env manually)

33

Can we add a link to Slack here?

yichendai added inline comments.
.github/PULL_REQUEST_TEMPLATE.md
10

These four items are writing prompts, not mandate requirement.

.github/PULL_REQUEST_TEMPLATE.md
10

My comment referred to an earlier version that said
"<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or fix a bug, please discuss it in an issue first: https://github.com/dagster-io/dagster/issues/new/choose -->
<!--- Please link to the issue here -->"
which did sound mandatory..

.github/PULL_REQUEST_TEMPLATE.md
10

Ooops, I prob skipped this comment and replied until I submitted an updated version 😓

.github/PULL_REQUEST_TEMPLATE.md
14
15

would remove this line

requesting a few changes. next time, might be better to do this in a quip doc for faster iteration?

.github/PULL_REQUEST_TEMPLATE.md
8
22

can we link directly to the contributors channel as we discussed?

24
25
This revision now requires changes to proceed.Jan 23 2021, 3:01 AM
yichendai marked 4 inline comments as done.

Update

.github/PULL_REQUEST_TEMPLATE.md
22

for some reason I didn't save that change. Update soon!

yichendai marked 2 inline comments as done.

Revise

catherinewu added inline comments.
.github/PULL_REQUEST_TEMPLATE.md
8
14
22
This revision now requires changes to proceed.Jan 26 2021, 2:58 AM
yichendai marked 3 inline comments as done.

Update

.github/PULL_REQUEST_TEMPLATE.md
22

The link in comment <!--- ---> is not clickable. I attached an example from Gatsby's issue template in Quip docs if you want to see how it looks like.

feel free to merge after making one last formatting change

.github/PULL_REQUEST_TEMPLATE.md
22

Thats true

22
This revision is now accepted and ready to land.Jan 26 2021, 6:47 AM
yichendai marked 2 inline comments as done.

Up and rebase

This revision was automatically updated to reflect the committed changes.