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

• [x] @pdpinch will propose an initial template
• [x] @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 :smile:

(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.

[pdpinch]

I opened a PR based on the discussion so far.

https://github.com/edx/edx-platform/pull/25949

I'm not sure where to host the meta "outcome" discussion that @nasthagiri describes. I'm putting it here, even though this may be less than ideal. Maybe it should be in Confluence?

Description the most import part of a PR, and somehow, often missing. There is a section to remind the author to write a description that explains what the PR does. The template text text explains why and hints that design decisions belong elsewhere.

JIRA or github tickets Links to tickets provide helpful context, but only when they are publicly accessible.

Discussions: Again, this is helpful for context. It also serves as a hint that public discussion on discuss.openedx.org is a good idea before opening a PR.

Dependencies: Of course, if there are dependencies, the reviewer needs to know. I'm not convinced that this isn't adequately covered by testing instructions though

Screenshots Reviewers who can't, or don't have the time, to spin up a devstack can still give valuable feedback based solely on screenshots. Including screenshots are also a signal that a review needs to consider UX.

Merge deadline This is a hint to the reviewer about how to prioritize their time.

Testing instructions One of the most critical parts of a PR description, but almost always overlooked.

Author Notes and concerns: This is a reminder to the author and the reviewer to be mindful of uncommon but complex cases, like migrations. I would have included annotations here, but I'm optimistic that they can be addressed with linting.

Commit Message and PR title I agree that guidlines on PR titles and commit messages should be part of this. But I'm not sure 1) how to establish the guidelines and 2) how to incorporate them into the PR template per se -- other than, perhaps, to add reviewing them as an item under author notes and concerns.

[nasthagiri]

@pdpinch Thanks for the PR. We can move our convo to the PR for further discussion on specific aspects of the template. (I suggested in the PR to use markdown-comments as the place for documenting the rationale/outcomes.) We can then discuss the necessity of each element there.

[nasthagiri]

For future reference, capturing this link (shared by @regisb) to a linter that can enforce conventional commits https://github.com/jorisroovers/gitlint

[nedbat]

I'm not sure merging edx/edx-platform#25949 should have closed this: there were other efforts mentioned in the description.

But, we do now have a PR template in the edx-platform repo.

[nasthagiri]

@nedbat I believe we've completed the first 2 tasks listed in the description: creating the template and adding the conventional-commits to it.

What's remaining at this point?

• [ ] @nedbat will provide edX-specific guidelines for conventional commits.
• [ ] Arbi-BOM's linter

Do you want to continue to use this issue for the next phase of this effort? Or call our Jan goal complete and open another issue for these follow-ups in Feb?

[sarina]

Clearing out old issues in some repos - this feels like it could be done. Thoughts?

[kdmccormick]

I think this needs some follow-up work:

• Evaluate whether folks are consistently using the template.
• If they are, move it to the .github repo so that all repos share the same default template (it's just edx-platform right now).
• If they're not, then either add linting to enforce the template, change the template, or remove it.

I'm in favor of making a new issue, linking it here, and then closing this issue.

[kdmccormick]

Here's a follow-up: https://github.com/edx/open-edx-proposals/issues/267

Closing this issue in favor of the new issue; if you disagree, let me know!