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

doc(rfc): Add requirements / non requirements section to RFC. #1818

Merged
merged 2 commits into from
Sep 14, 2020
Merged

Conversation

jplaisted
Copy link
Contributor

@jplaisted jplaisted commented Aug 24, 2020

This seems helpful to reviewers (and probably also the author). Motivation is too high level to be able to properly review a design. We need a clear set of requirements to review the design against.

Stems from comments on #1778 (review)

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

This seems helpful to reviewers (and probably also the author). Motivation is too high level to be able to properly review a design. We need a clear set of requirements to review the design against.
> Once everyone has agreed upon the set of requirements for your design, we can use this list to review the detailed
> design.

### Extensibility
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a sub-section of requirements? Shouldn't it be top level or a sub-section of detailed design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extensibility requirements? I'm fine either way, but this a little clearer is it still a list of requirements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have this as a separate section as part of "Future Work".

@@ -21,6 +21,28 @@
> alternative solutions. In other words, enumerate the constraints you are trying to solve without coupling them too
> closely to the solution you have in mind.

## Requirements
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think Goals & Non-goals would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, those can be too high level imo. "Goals" says high level, "requirements" says specific, to me at least.

For most designs I really think we need a detailed, bulleted list. We need to consider the design's requirements/goals in detail from the outset, and in plain English (outside of detailed design).

Copy link
Contributor

@mars-lan mars-lan left a comment

Choose a reason for hiding this comment

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

Let's also add a deadline section? Depending on the size of RFC, we can have 1 week for small, 2 weeks for medium, and 4 weeks for large. The reviewers can request extensions (maybe 1 week at a time?) if the discussion isn't converging. This way we can move RFCs forward at a more predictable pace.

@jplaisted
Copy link
Contributor Author

Let's also add a deadline section? Depending on the size of RFC, we can have 1 week for small, 2 weeks for medium, and 4 weeks for large. The reviewers can request extensions (maybe 1 week at a time?) if the discussion isn't converging. This way we can move RFCs forward at a more predictable pace.

Should that be in the RFC or in the PR/issue? Either way probably involves updating the process doc. And can be done in a different PR :)

@mars-lan
Copy link
Contributor

Let's also add a deadline section? Depending on the size of RFC, we can have 1 week for small, 2 weeks for medium, and 4 weeks for large. The reviewers can request extensions (maybe 1 week at a time?) if the discussion isn't converging. This way we can move RFCs forward at a more predictable pace.

Should that be in the RFC or in the PR/issue? Either way probably involves updating the process doc. And can be done in a different PR :)

Ideally both, but definitely for RFC. PR should really just be a straight forward realization of RFC and should be less controversial when it comes to review time.

@jplaisted
Copy link
Contributor Author

Let's also add a deadline section? Depending on the size of RFC, we can have 1 week for small, 2 weeks for medium, and 4 weeks for large. The reviewers can request extensions (maybe 1 week at a time?) if the discussion isn't converging. This way we can move RFCs forward at a more predictable pace.

Should that be in the RFC or in the PR/issue? Either way probably involves updating the process doc. And can be done in a different PR :)

Ideally both, but definitely for RFC. PR should really just be a straight forward realization of RFC and should be less controversial when it comes to review time.

I think my concern is discoverability. I don't want an RFC opened with a week deadline and then just an immediate submit a week later. So if you aren't getting comments, I'm not sure putting it in the review is the best place to put it? I think we need more of a process around it. So I think we can do this in a different PR.

@jplaisted jplaisted merged commit fbb5230 into datahub-project:master Sep 14, 2020
@jplaisted jplaisted deleted the rfc branch February 26, 2021 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants