There is limited direct or obvious support for working with a stack of
dependent pull requests with clean code review of each incremental
change.
Carbon needs to support high-latency asynchronous code review due to
timezones, schedule differences (especially in an open source project),
and the delays imposed by our proposal process. To this end we need some
solution for doing stacked pull requests with a reasonable code review
experience. This suggests a compromise flow that seems to minimize the
costs of doing this.
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
Co-authored-by: Dmitri Gribenko <gribozavr@gmail.com>
Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
Co-authored-by: austern <austern@google.com>
The pull request workflow was supposed to connect to the review guidance
anyways, but the link hadn't been added.
Also adds a TOC to the pull request workflow document as it was missing
one.
Carbon needs a strong code review process to handle code (and other)
changes that are not significant proposals. This attempts to provide
clear guidance on the process, structure, and scope of code review.
It also gives detailed guidance on how to effectively do code review
for both reviewers and authors.
This proposal was accepted on 2020-08-04.
This had been causing me issues when writing a test for pr-comments.py, which I then renamed so that I could import. It's had me mulling that I should really write tests for other scripts, but the name gets in the way.
The decision files are just an exception, so trying to reach consistency.
- Add mentions of new "comment deadline" label.
- Link to review manager doc from evolution doc.
- Standardize evolution doc on "topic" instead of "forum topic" (use was mixed).
We're running into issues with md handlers that expect this kind of 4-space indent. It should be cross-compatible with GH, so switch, even though it feels a little churny.
Manual edits are to:
- .prettierrc.yaml:
- rename from .prettierrc
- add tabWidth (primary change)
- add trailingComma (fix vimPrettier skew)
- contribution_tools.md: Fix remarks about .prettierrc.yaml
- pre-commit-toc.js: indent, bullets
- pre-commit-proposal-list.py: indent of output
The rest is the result of `pre-commit run --all-files`
Unfortunately this'll probably depend on the change being propagated into PR branches, so I wouldn't be surprised if we see regressions for a bit. We'll also need to nudge people to update .vimrc's. Hopefully the pre-commit GH action helps catch issues.
* Proposal for an explicit GitHub workflow.
This suggests a GitHub workflow that uses pull requests, produces linear
history, and both incentivizes and encourages small, incremental changes
(both at the pull request and commit granularity). It tries to follow
general best practices around software engineering at scale and GitHub
workflows. It also tries to ensure the workflow is very well supported
by tooling and automation built into GitHub.
Of note, this proposal should match precisely the current enforced flow
on our GitHub repositories. But we need to actually decide we like this,
write up the rationale behind it, and document what we're doing.
I've added an abbreviated version of the proposal as a documentation
update to the contributing file. Happy to restructure or find a better
home for this. I've tried to focus on the parts that contributors
actually would need to care about as opposed to the things that are
simply and fully enforced mechanically.
My hope after this is to suggest more detailed code review guidelines.
* Addressing review comments.
Notably, I really was giving too much weight to multi-commit PRs which
shouldn't be the common or default. I've tried to restructure everything
to make it much more clear what is going on here.
* Minor tweaks
* Fix typo in CONTRIBUTING.md
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
* Extract workflow description and remove redundancies.
* remove tracking issue template field
* Update proposals/p0029.md with reviewer suggsetion.
Co-authored-by: Dmitri Gribenko <gribozavr@gmail.com>
* Continue to address review feedback.
* Apply suggestions from code review
Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
Co-authored-by: austern <austern@google.com>
* Incorporate code review feedback and begin moving toward "trunk" based terminology
* Tweak the wording and make it a bit more consistent.
* Update docs/project/pull_request_workflow.md
Co-authored-by: Dmitri Gribenko <gribozavr@gmail.com>
* Address more review comments.
* Correct the rationale.
* Tweak wording based on discussion in review.
* Improve the rationale around the default branch to avoid overstating or
misstatig things.
* Apply suggestions from code review
Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
* Clean up formatting and address a couple of comments on grammar from review.
* Update docs/project/pull_request_workflow.md
Co-authored-by: austern <austern@google.com>
* Add to proposal list.
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
Co-authored-by: Dmitri Gribenko <gribozavr@gmail.com>
Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
Co-authored-by: austern <austern@google.com>
This catches some common spelling errors, but is *not* an exhaustive spell-checker. The only false positive is "rouge" in Gemfile.lock, which is why I'm excluding that file.
True positives are in the change, and it does catch the "langauge" typo I keep making, which zygoloid fixed for me in a few spots. False negatives are:
- virmc
- Announcemented
- propsal
For comparison:
- cspell (https://www.npmjs.com/package/cspell) is too aggressive and has a small dictionary
- Caught the false negatives, but many false positives, including 'LLVM', 'roadmap', 'Carruth'
- https://github.com/lorenzwalthert/precommit has a spellcheck, but config looks ruby-specific
Added here:
- .pre-commit-config.yaml
- docs/project/contribution_tools.md
- src/scripts/pre-commit-toc.js
- CONTRIBUTING.md
- proposals/* (just to switch TOC formats)
The rest consists of auto-generated fixes.
The switch from doctoc to markdown-toc is because doctoc wants to put a TOC in every file, whereas markdown-toc only does it if you have an insertion point.
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
- Sentence case headers: https://developers.google.com/style/headings
- Remove parentheticals where possible: https://developers.google.com/style/parentheses
- Avoid latin abbreviations: https://developers.google.com/style/abbreviations#dont-use
Also some evolution-specific changes:
- Remove obsolete PDF references
- "decision is to accept the proposal" instead of "decision is to approve the change"
- Adjust review manager actions for accepted/declined/deferred decisions.
- Rephrase arbiter decisions, particularly to avoid 1-0 being considered a "majority".
- I think this was always intended, and does not reflect a substantive change.
* First draft of a README for the principles/ directory.
* More closely mirror the relevant section from the goals doc.
* Reword bit about excluding using chandlerc@'s suggested text.
* Link to goals doc.
* Update docs/project/principles/README.md
Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
The original header was exactly 80-columns which made every file format
with a line-start comment syntax reflow the text. Instead, pick
a canonical re-flowing that keeps it to just three lines. This also lets
the text refer to the project which seems more clear.
No practical or semantic change here, just getting consistent wording
and line breaks for regularity.
CONTRIBUTING.md replaces the mention of Google style guide (which isn't automated) with Prettier (which should do most of the hauling). The rest is the formatter.
Richard Smith
Jon Meow
George Kulakowski
sidney13