dev docs: update guidance on code changes#37274
Conversation
martykulma
force-pushed
the
maz-pr-guidelines-update
branch
from
f07bdb4 to
d2e7a3d
Compare
martykulma
force-pushed
the
maz-pr-guidelines-update
branch
from
d2e7a3d to
8442648
Compare
| * **[Self-review and pass CI](#polish-requirements)** before requesting | ||
| review; use draft PRs for early feedback. |
There was a problem hiding this comment.
I think default to opening all PRs in draft until you have done a self review and PR checks are green is a solid best practice.
Should we suggest that people consider using LLMs to aid with self review here, we do have a pr review skill.
| * **[Keep PRs small](#pr-size-limits)** — ideally under 500 lines; reviewers | ||
| often ask to split PRs over 1000. Limit each RU to one team's | ||
| [CODEOWNERS](/.github/CODEOWNERS) scope. |
There was a problem hiding this comment.
Maybe we should make a note about "ideally under 500 lines of behavioral changes"? For instance, #37260 is a recent refactor that has about ~500 lines of code that needs paying attention to, split by behavioral / refactoring. However, it adds about 1000 lines of simple one line changes. Another instance are Console PRs where the majority of the changes are markup (JSX, html).
There was a problem hiding this comment.
Agree. The 500 lines is not a hard limit, they are targets because exceptions will always exist. I'll update to call that out.
I would say that PR should have had refactoring split out 😏
- PR1: unify authZ
- PR2: fix allowed_roles
Further down, I added a section on refactoring + behavioral changes (~ln:169) that suggests keeping them separate as a way to allow the reviewers to focus on the behavior changes.
There was a problem hiding this comment.
Valid take! After reading this doc, I think I'll split out my refactors by PR rather than commit from now on 💯
| description reads "do X *and* Y *and* Z," split it. Never mix refactoring and | ||
| behavior changes in the same RU. | ||
|
|
||
| * **[Keep PRs small](#pr-size-limits)** — ideally under 500 lines; reviewers |
There was a problem hiding this comment.
Not clear if "500 lines" means 500 lines of added code, or 500 lines of added+deleted code
There was a problem hiding this comment.
It should be 500 lines of changes. Will update!
mtabebe
left a comment
mtabebe
left a comment
There was a problem hiding this comment.
Thanks for this thoughtful update
| change. For a project, that assumption can be flipped. Pair an implementer with | ||
| a dedicated reviewer/partner from the start: one writes the feature, the other | ||
| reviews it and stays close to the design throughout. When the reviewer already | ||
| shares your context, a larger diff is no longer overwhelming, so the pair can |
There was a problem hiding this comment.
I don't agree that working with someone means larger diffs are no longer overwhelming. Also just because one person is working with someone on a project doesn't mean that others also won't be participating in reviews
There was a problem hiding this comment.
yikes, can't believe I left that in there. absolutely - i'll reword that.
| arrived PRs at least once a day. Beyond that the throughput will vary: some days | ||
| you'll clear five or six PRs, other days you'll clear none, and both are fine. | ||
|
|
||
| When you have several PRs in your queue and can't get to them all at once, weigh |
There was a problem hiding this comment.
Thanks for asking the questions, it motivated me think more on what these 2 axes were!
|
Don't wanna bloat our PR guidelines and make it too verbose, but one thing I've seen people do and have adopted myself is when using AI agents to create a PR, adding a commit to publish the plan, then adding a commit to remove the plan at the end. At least as a reviewer, it gives me high level context of what's going on and as the reviewee, easy to compile if ran under a single AI session. Maybe worth putting under "### AI-assisted review" as a note like "It's encouraged to commit the generated plan for the session in a commit as long the plan is removed in another commit at the end"? |
Alphadelta14
left a comment
Alphadelta14
left a comment
There was a problem hiding this comment.
Approving for generally good value, reasonable expectations, direct actions.
I'll do as i can to continually refer back to this from time to time to refresh my knowledge as i review/submit changes.
5 participants
Motivation
Authoring and reviewing changes are adjusting as AI usage increases and our PR workflows evolve (hello stacked PRs). This updates Materialize developer guidelines regarding code changes to address that evolution.
Description