**PR Template.** As a PR reviewer, I would like contributors to use a PR template, in order to establish standardization and base expectations.

[nasthagiri]

Next Steps

• @pdpinch will propose an initial template
• @nedbat @nasthagiri will update to include recommendations for PR descriptions and commit messages (unless @pdpinch beats her to it)
• @jmbowman and the Arbi-BOM team will look into developing linters for tight feedback loops on PR best practices

[antoviaque]

For reference, here is OpenCraft's PR template: https://handbook.opencraft.com/en/latest/pull_request_process/#what-to-include-in-a-pull-request

[pomegranited]

Are there any fields missing from OpenCraft's PR template? Anything that should be removed?

[pdpinch]

It covers most of the bases -- including a note about flagging migrations. I think the only thing I might add is a similar note about adding feature flags. But that may be overkill.

[pomegranited]

@pdpinch It's easy to see if a PR affects migrations, since it'll have files added under a migrations folder, and so this should be caught during code review. Do we need to call them out in the PR description too?

Re feature flags -- maybe a more general step for ensuring that changes are appropriately documented? OpenCraft have a template we use for code reviews which we use to catch this, though it's hazy as to where exactly the change should be documented -- In the code? In an ADR? A README? A wiki somewhere? The edX user docs? But most changes will require some form of documentation change.

In the case of feature flags, the edX Feature Flags wiki may be the most up-to-date source of info we have.

[pdpinch]

I agree that the reviewer should be responsible for pointing out any shortcomings with migrations of feature flags. But in practice, the detail work necessary for these changes often gets overlooked. For migrations, we need reverse migrations and possibly a devops review. For feature flags we need the necessary annotations.

Can we expect any given combination of contributor and reviewer to remember these details?

[kdmccormick]

@pomegranited : Re feature flags -- maybe a more general step for ensuring that changes are appropriately documented?

I'm glad you asked! We've been encouraging annotation of new feature flags in accordance with OEP-17, and @regisb and @robrap are doing work to improve things further. All new toggles, Django or Waffle, should be annotated similar to this one. If the template has a sort of "things to check for in review" list, I recommend including feature toggle annotations 😄

(Btw, that feature flag wiki page is quite out-of-date now! I'll add a warning at the top of the page.)

@pdpinch : I agree that the reviewer should be responsible for pointing out any shortcomings with migrations of feature flags. But in practice, the detail work necessary for these changes often gets overlooked. For migrations, we need reverse migrations and possibly a devops review. For feature flags we need the necessary annotations. Can we expect any given combination of contributor and reviewer to remember these details?

+1 -- in absence of good training and/or docs, details are likely to get overlooked. I think we should be able to expect code reviewers and contributors to remember such points, if and only if we provide them onboarding and reference materials that teach them so.

That being said: the more we add to the template, the more likely it'll be ignored. I think we have to choose our battles in regards to what information is so vital that it needs to be by-default pasted into every pull request. I like the OpenCraft template because it's not too long, captures the vital points, and is more of a "fill-me-in" template than a "read-these-bullet-points" template, the latter of which I think will be quickly ignored.

Now, although I would avoid filling up the PR template with generic coder/reviewer considerations, I'd still advocate for developing a centralized "How to Review Code" doc somewhere (like a new page in the edX developer docs) and then eventually linking out to that in the PR template. OpenCraft's Code Reviewer Expectations strikes me as a good model for such a page. That way, we could keep the templates lean while having a detailed and de-duped guide for reviewers to refer to as needed.

So, I would be in favor of adopting the OC template, with three recommendations:

• Remove Sandbox URL -- edX.org generally only uses sandboxes for PRs that'd be impossible to test in devstack, and our sandboxes are only available internally. For PRs that do spin up sandboxes, they could probably be mentioned in Testing Instructions.
• Add 3. Did you annotate any new feature flags? under Author Notes and Concerns. But I do not feel super strongly about this -- maybe it belongs to the next bullet point.
• Strive to develop a "How to Review Code" guide in our developer documentation. When it's ready, include a link to it the PR template.

Finally, for any PR template to be successful, there needs to be a culture of actually caring about the template and using it. We'll want to:

• circulate the decision among CCs and CC champions, and encourage them to enforce template using when reviewing code, and
• reach out to engineers and managers at edX and ask them to do the same. With @nasthagiri 's support I can probably take this on.

[robrap]

Regarding feature flag annotations, the intention is to add linting. Lists can be helpful, but linting (or architectural fitness functions) should be preferred when it is efficient, accurate, and helpful.

[nasthagiri]

All, 🙇‍♀️ Thanks for the discussion thus far. I'm overall supportive of moving forward with a generalized adaptation of OC's PR template.

Simple beats Complex (link to entry in Arch Manifesto)

Since each item takes on valuable real estate and cognitive load for each and every PR (for authors and reviewers), each item bears a cost of investment. I like the notion of keeping the PR template short and simple - with links to other references for more detail (as @kdmccormick recommends).

One way to achieve this is to align on the outcome for each item in the template. For example, we can externalize/document the outcome for each item as a comment in the PR Template doc. By "outcome", I mean the goal we are striving for - what exactly we are hoping to prevent or achieve. What do you think?

Automated Fitness Functions beat Manual Verifications (link to entry in Arch Manifesto)

I'd like to have us create automated fitness functions whenever possible rather than rely on PR template checklists (as @robrap points out above).

• @robrap Do you and @regisb have plans for a Feature Toggle annotation linter as part of the current Blended project?
• For Migrations, what in particular is the outcome we strive for? Can we automate this detection with a GitHub Action or a Python linter? I can then work with @jmbowman to have Arbi-BOM implement this for us (unless someone in the community would like to jump on it).

Semantic Commit Messages

Note that as part of the rollout of this effort, I'd like to standardize naming of our PR titles and commit messages. I am inspired by:

• This commit history in docusaurus as described in their contributing doc.
• Angular's commit history follows a similar convention as described in their developers doc.
• They are then able to automate the creation of more precise change logs for their repos.

The outcomes of this change being:

• Readability of history: A more readable commit history
• Discipline of PR creations: When developers externalize the intent of their change, they improve the decomposition of their changes. (For example, keeping refactoring commits separate from feature enhancements.)
• Precise Changelogs: Having this process be part of each and every commit and PR, there's less risk of forgetting to update a separate changelog.

So I'd like to move forward with this change as part of our PR fitness functions: template (manual checklist) and automation (linters/CI).

[robrap]

@nasthagiri: Yes, I’m hoping we get at least minimal linting from the blended toggles project.