Proposal for an explicit GitHub workflow. #29

chandlerc · 2020-05-27T10:24:41Z

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.

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.

CONTRIBUTING.md

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.

chandlerc

Thanks for the feedback. Hopefully really clarified the whole multi-commit thing, and refocused everything on the fact that in most cases a single squashed commit is all you'll want or need.

CONTRIBUTING.md

proposals/p0029.md

Co-authored-by: josh11b <josh11b@users.noreply.github.com>

chandlerc · 2020-06-02T02:13:19Z

Restructured significantly to make it more clear how this could meaningfully land. PTAL.

docs/project/pull_request_workflow.md

proposals/p0029.md

Co-authored-by: Dmitri Gribenko <gribozavr@gmail.com>

chandlerc

Responding to review comments, thanks for all of them!

proposals/p0029.md

docs/project/pull_request_workflow.md

proposals/p0029.md

…review.

docs/project/pull_request_workflow.md

Co-authored-by: austern <austern@google.com>

jonmeow

Approving per decision: https://forums.carbon-lang.dev/t/accepted-linear-rebase-and-pull-request-github-workflow/88

…100) - [Proposal PR](#29) - [RFC](https://forums.carbon-lang.dev/t/rfc-use-a-linear-pull-request-github-worflow/53) - [Decision request](https://forums.carbon-lang.dev/t/request-for-decision-proposal-for-an-explicit-github-workflow-29/78) - [Decision announcement](https://forums.carbon-lang.dev/t/accepted-linear-rebase-and-pull-request-github-workflow/88)

Only change is to update the path to the fuzzer build extension. Original main commit message: > Add an initial parser library. (#30) > > This library builds a parse tree, very similar to a concrete syntax > tree. There are no semantics here, simply introducing the basic > syntactic structure. > > The current focus has been on the APIs and the data structures used to > represent the parse tree, and not on the actual code doing the > parsing. The code doing the parsing tries to be reasonably efficient > and reasonably easy to understand recursive descent parser. But there > is likely much that can be done to improve this code path. A notable > area where very little thought has been given yet are emitting good > diagnostics and doing good recovery in the event of parse errors. > > Also, this code does not try to match the current under-discussion > grammar closely. It is only partial and reflects discussions from some > time ago. It should be updated incrementally to reflect the current > expected grammar. > > The data structure used for the parse tree is unusual. The first > constraint is that there is a precise one-to-one correspondence > between the tokens produced by the lexer and the nodes in the parse > tree. Every token results in exactly one node. In that way, the parse > tree can be thought of as merely shaping the token stream into a tree. > > Each node is also represented with a fixed set of data that is densely > packed. Combined with the exact relationship to tokens, this allows us > to fully allocate the parse tree's storage, and to use a dense array > rather than a pointer-based tree structure. > > The tree structure itself is implicitly defined by tracking the size > of each subtree rooted at a particular node. See the code comments for > more details (and I'm happy to add more comments where necessary). The > goal is to minimize both the allocations (one), the working set size > of the tree as a whole, and optimize common iteration patterns. The > tree is stored in postorder. This allows depth-first postorder > iteration as well as topological iteration by walking in reverse. > > Building the parse tree in postorder is a natural consequence of the > grammar being LR rather than LL, which is a consequence of supporting > infix operators. > > As with the Lexer, the parser supports an API for operating on the > parse tree, as well as the ability to print the tree in both > a human-readable and machine-readable format (YAML-based). It includes > significant unit tests and a fuzz tester. The fuzzer's corpus will be > in a follow-up commit. > > This is the largest chunk of code already written by several of us > prior to open sourcing. (There are a few more pieces, but they are > significantly smaller and less interesting.) If there are major things > that folks would like to see happen here, it may make sense to move > them into issues for tracking. I have tried to update the code to > follow the style guidelines, but apologies if I missed anything, just > let me know. We also have issues #19 and #29 to track things that > already came up with the lexer. Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>

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

Only change is to update the path to the fuzzer build extension. Original main commit message: > Add an initial parser library. (#30) > > This library builds a parse tree, very similar to a concrete syntax > tree. There are no semantics here, simply introducing the basic > syntactic structure. > > The current focus has been on the APIs and the data structures used to > represent the parse tree, and not on the actual code doing the > parsing. The code doing the parsing tries to be reasonably efficient > and reasonably easy to understand recursive descent parser. But there > is likely much that can be done to improve this code path. A notable > area where very little thought has been given yet are emitting good > diagnostics and doing good recovery in the event of parse errors. > > Also, this code does not try to match the current under-discussion > grammar closely. It is only partial and reflects discussions from some > time ago. It should be updated incrementally to reflect the current > expected grammar. > > The data structure used for the parse tree is unusual. The first > constraint is that there is a precise one-to-one correspondence > between the tokens produced by the lexer and the nodes in the parse > tree. Every token results in exactly one node. In that way, the parse > tree can be thought of as merely shaping the token stream into a tree. > > Each node is also represented with a fixed set of data that is densely > packed. Combined with the exact relationship to tokens, this allows us > to fully allocate the parse tree's storage, and to use a dense array > rather than a pointer-based tree structure. > > The tree structure itself is implicitly defined by tracking the size > of each subtree rooted at a particular node. See the code comments for > more details (and I'm happy to add more comments where necessary). The > goal is to minimize both the allocations (one), the working set size > of the tree as a whole, and optimize common iteration patterns. The > tree is stored in postorder. This allows depth-first postorder > iteration as well as topological iteration by walking in reverse. > > Building the parse tree in postorder is a natural consequence of the > grammar being LR rather than LL, which is a consequence of supporting > infix operators. > > As with the Lexer, the parser supports an API for operating on the > parse tree, as well as the ability to print the tree in both > a human-readable and machine-readable format (YAML-based). It includes > significant unit tests and a fuzz tester. The fuzzer's corpus will be > in a follow-up commit. > > This is the largest chunk of code already written by several of us > prior to open sourcing. (There are a few more pieces, but they are > significantly smaller and less interesting.) If there are major things > that folks would like to see happen here, it may make sense to move > them into issues for tracking. I have tried to update the code to > follow the style guidelines, but apologies if I missed anything, just > let me know. We also have issues #19 and #29 to track things that > already came up with the lexer. Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>

jonmeow reviewed May 27, 2020

View reviewed changes