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

Improve description of what belongs in a proposal document versus in the design #649

Merged
merged 9 commits into from
Jul 26, 2021
Merged

Improve description of what belongs in a proposal document versus in the design #649

merged 9 commits into from
Jul 26, 2021

Conversation

zygoloid
Copy link
Contributor

Expand the description of the structure of a proposal PR. Clarify that the full PR is the proposal, not only the P-numbered document. Start a design style guide and use it to describe which parts of a proposal should not end up in the design.

@zygoloid zygoloid requested review from a team as code owners July 14, 2021 01:50
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Jul 14, 2021
…the design

Expand the description of the structure of a proposal PR. Clarify that
the full PR is the proposal, not only the P-numbered document. Start a
design style guide and use it to describe which parts of a proposal
should not end up in the design.
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Some suggestions, I think I understand what you're getting at, but I'm having trouble parsing the wording.

docs/project/design_style_guide.md Outdated Show resolved Hide resolved
docs/project/design_style_guide.md Outdated Show resolved Hide resolved
docs/project/design_style_guide.md Outdated Show resolved Hide resolved
docs/project/design_style_guide.md Outdated Show resolved Hide resolved
docs/project/evolution.md Outdated Show resolved Hide resolved
docs/project/evolution.md Outdated Show resolved Hide resolved
zygoloid and others added 3 commits July 21, 2021 11:47
Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

LGTM

docs/project/evolution.md Outdated Show resolved Hide resolved
docs/project/evolution.md Outdated Show resolved Hide resolved
docs/project/design_style_guide.md Outdated Show resolved Hide resolved
docs/project/design_style_guide.md Outdated Show resolved Hide resolved
docs/project/evolution.md Outdated Show resolved Hide resolved
docs/project/design_style_guide.md Outdated Show resolved Hide resolved
zygoloid and others added 2 commits July 21, 2021 14:37
Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
docs/design/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

FWIW, this is really nice, and thanks for writing it up.

Comments below are honestly pretty minor. If they make sense, land it. If I've just missed something, let me know and land it. Either way, LGTM. Of course, if there is something confusing here, of course, happy to chat and clairfy.

I'm also approving as I think this is pretty much clarifying the process and not a significant change. As a result, I think its fine to go ahead and land and iterate here. But we should definitely iterate if @KateGregory has any other feedback as a lead -- I think its good for all three of us to have read this in a reasonable amount of detail and be happy.

docs/design/README.md Outdated Show resolved Hide resolved
docs/project/design_style_guide.md Outdated Show resolved Hide resolved
docs/project/design_style_guide.md Outdated Show resolved Hide resolved
Comment on lines 93 to 94
- Proposal [#123](https://github.com/carbon-language/carbon-lang/pull/123):
widget painting.
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor)

Again guessing we should use a large linked text to be accessible / easy to find. (That said, I'm not an expert and am just guessing -- maybe I'm wrong about what is most useful here?)

Suggested change
- Proposal [#123](https://github.com/carbon-language/carbon-lang/pull/123):
widget painting.
- [Proposal #123: Widget painting.](https://github.com/carbon-language/carbon-lang/pull/123)

Copy link
Contributor

@jonmeow jonmeow Jul 23, 2021

Choose a reason for hiding this comment

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

FWIW, it might be worth thinking here about what we could, in theory, automate:

  • We can definitely automate detecting #123 and suggesting [#123](https://github.com/carbon-language/carbon-lang/issues/123), just turning it into a link.
    • Note here that linking issues instead of pull allows it to be either an issue or a PR. This kind of link works in both cases.
  • We can sort of automate suggesting [#123: Widget painting](https://github.com/carbon-language/carbon-lang/pull/123) by detecting the #123 in a link, looking for the corresponding proposal file, reading the title, etc. This may be fragile if somebody points at an in-flight proposal (or issue).
    • It couldn't accurately capture surrounding text, e.g. if somebody wrote #123: the painting proposal without a link and with a non-matching description. You'd end up with [#123: Widget painting](...): the painting proposal.
  • We could also get into github APIs and read from the server for something more durable, at the cost of creating server RPCs as part of formatting, but I worry about running repeated server RPCs from pre-commit. I'd advise against this.
  • Formatting as suggested here would require formatting any link matching .*\W#\d+\W.* which may be a little too flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I've gone with for now is to make including the title of the proposal optional in the link text. For automation purposes, we can detect existing links of the form "[#NNN..." and make sure they link to the right place, and can form links from text #NNN, but can't easily include a following ": ..." in the link nor automatically add one. We can probably do better, but I think we can handle any further improvement incrementally from this starting point.

@zygoloid zygoloid merged commit c2b5077 into carbon-language:trunk Jul 26, 2021
proposal would then be implemented through a series of future PRs to the rest of
the project, and the proposal document should describe what is being proposed in
enough detail to validate that those future PRs properly implement the proposed
direction.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should have some statement explicitly saying that the quality standard for proposal documents are lower.

@zygoloid zygoloid deleted the evolution-what-is-a-proposal branch March 11, 2022 01:01
chandlerc added a commit that referenced this pull request Jun 28, 2022
…n versus in a proposal document. (#649)

Expand the description of the structure of a proposal PR. Clarify that
the full PR is the proposal, not only the P-numbered document. Start a
design style guide and use it to describe which parts of a proposal
should not end up in the design.

Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants