Details

Reviewers

• max
• catherinewu
alangenfeld

Commits

R1:1e3d6c468446: [Github Community] Add a PR template

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:

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

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 edited the summary of this revision. (Show Details)Dec 17 2020, 12:57 AM

• yichendai retitled this revision from [RFC]Add a PR template to [Github Community] Add a PR template.Dec 17 2020, 1:00 AM

Harbormaster completed remote builds in B23064: Diff 28045.Dec 17 2020, 1:07 AM

• yichendai requested review of this revision.Dec 17 2020, 1:07 AM

• yichendai edited the summary of this revision. (Show Details)Dec 17 2020, 1:18 AM

• yichendai added reviewers: alangenfeld, • max.

Update: - Refactor

Harbormaster completed remote builds in B23238: Diff 28255.Dec 21 2020, 12:27 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

Harbormaster completed remote builds in B23271: Diff 28303.Dec 21 2020, 7:40 PM

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 a subscriber: alangenfeld.Jan 9 2021, 12:03 AM

• 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.

Update PR template

Harbormaster failed remote builds in B23995: Diff 29178!Jan 9 2021, 12:49 AM

• catherinewu added inline comments.Jan 9 2021, 12:50 AM

.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?

Modify PR template

• yichendai marked 6 inline comments as done.Jan 9 2021, 2:54 AM

• yichendai added inline comments.

.github/PULL_REQUEST_TEMPLATE.md
10	These four items are writing prompts, not mandate requirement.

• yichendai marked an inline comment as done.Jan 9 2021, 2:56 AM

Harbormaster completed remote builds in B24009: Diff 29195.Jan 9 2021, 3:05 AM

• catherinewu added inline comments.Jan 11 2021, 5:50 PM

.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..

• yichendai added inline comments.Jan 11 2021, 5:59 PM

.github/PULL_REQUEST_TEMPLATE.md
10	Ooops, I prob skipped this comment and replied until I submitted an updated version 😓

• yichendai marked an inline comment as done.Jan 11 2021, 6:01 PM

• yichendai added a reviewer: • catherinewu.Jan 15 2021, 8:48 PM

• yichendai added a child revision: D6025: [Documentation] Refresh the Contributing guides.

Revise summary

• yichendai edited the summary of this revision. (Show Details)Jan 21 2021, 12:43 AM

Harbormaster completed remote builds in B24624: Diff 29986.Jan 21 2021, 12:43 AM

• catherinewu added inline comments.Jan 21 2021, 2:38 AM

.github/PULL_REQUEST_TEMPLATE.md
14
15	would remove this line

Revise

Harbormaster completed remote builds in B24626: Diff 29989.Jan 21 2021, 4:11 AM

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

Add contributors channel

Update

• yichendai added inline comments.Jan 23 2021, 5:15 AM

.github/PULL_REQUEST_TEMPLATE.md
22	for some reason I didn't save that change. Update soon!

Harbormaster completed remote builds in B24756: Diff 30148.Jan 23 2021, 5:28 AM

Harbormaster completed remote builds in B24757: Diff 30149.Jan 23 2021, 5:31 AM

Revise

Harbormaster completed remote builds in B24818: Diff 30222.Jan 25 2021, 6:58 PM

Harbormaster completed remote builds in B24819: Diff 30223.Jan 25 2021, 7:03 PM

• catherinewu requested changes to this revision.Jan 26 2021, 2:58 AM

• 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

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.