VACMS-16409: Update project-conventions.md
#16410
Conversation
ndouglas
requested review from
BerniXiongA6,
EWashb,
maortiz-27-80 and
JunTaoLuo
|
|
||
| * As a reviewer, use [conventional comments](https://conventionalcomments.org/) to signal your intent, e.g. **Nitpick**, **Suggestion (non-binding)**, **Request**, etc. | ||
| * Do not make unsolicited comments on PRs that do not belong to a member of your team unless you feel it poses a clear and present danger. | ||
| * If you have a suggestion for an alternative approach, or a concern about the ticket or the approach, escalate it to your PM/DM/PO. |
There was a problem hiding this comment.
Though: I'm a little skeptical of this requirement as it may be a little harsh. I understand we want to limit the amount of unhelpful or irrelevant comments but I don't also don't want to add more roadblocks to collaboration which can lead to more siloing of knowledge and expertise. However, there should be the expectation that if you make unsolicited comments, the PR owner is very likely or expected to dismiss or reject the comments.
There was a problem hiding this comment.
This is to prevent interference with an engineer that is attempting to address a ticket as it is written.
In other words, if I write a ticket with ACs A, B, and C, we all prefine it and agree that it is fine, we refine it and agree it is fine, and then Engineer Dave takes this on, and then Engineer Frank from another team comes along and says "no no, you don't want to do A, B, and C, you need to do D, E, and F!"
I believe that is inappropriate. If Frank believes that the ticket or PR is taking the wrong approach, I think this is a matter that needs to involve Dave's PO, PM, DM, and TL, and Frank's PM and DM. It will (or should) result in cancelling or rewriting or re-refining a ticket. It's a big deal, in my opinion, and should be mediated.
I don't wish to prevent or inhibit collaboration. I do want to ensure that engineers have space in which to do their work without unwanted interference, especially interference on the level of requirements/definition when they're trying to work on implementation.
Perhaps I can adjust this to where we can have both? Maybe I didn't have the concept clearly defined in my mind when I wrote this. Any suggestions on how I can improve it?
There was a problem hiding this comment.
Ah I see, I misunderstood it as only PM/DM/POs are allowed to comment on PRs to suggest alternative approaches. How about updating the wording to this?
| * If you have a suggestion for an alternative approach, or a concern about the ticket or the approach, escalate it to your PM/DM/PO. | |
| * If you have a suggestion for an alternative approach, or a concern about the ticket or the approach, you must also include your PM/DM/PO in your comment and ensure they approve the modifications to the original ticket or approach. |
There was a problem hiding this comment.
Here are some examples of what I mean:
Example 1: Possibly bad ticket.
- Dave's ticket is changing an instruction on a form -- for a particular field.
- Frank sees this and thinks, gee, that wording is really crappy/not clear/not inclusive/etc.
- What's the wording in the original ticket? Well, it's exactly the wording that Dave is trying to use.
- In other words, Dave might move forward to review and merge something bad. Not actively destructive, but not good.
- My opinion is that Frank should raise this to product or delivery peeps in his own team before proceeding.
I don't mean that only a PM/DM can comment, but that this sort of communication needs to at least be cleared by Frank's PM/DM prior to commenting. Otherwise it just seems like interference to me, not collaboration.
Example 2: Good ticket, bad approach.
- Dave's ticket includes some kind of algorithm, and his implementation is O(n^2) over a large number of items when it'd be trivial to do it in O(n).
- Frank sees this and thinks, what in the
- The ticket does not say "Do this in the least efficient way possible."
- In other words, Dave might move forward to review and merge something bad.
- My opinion is that Frank is free to step in and (kindly, politely) suggest a better performing approach without first raising it to PM/DM.
I'd argue that also includes "hey, you can save five LOC by doing X". Ideally, we always love to learn and welcome a tactfully-offered tweak to our technique.
There was a problem hiding this comment.
Yes that does clarify it up a lot, thanks. I wanted to make sure we continue to promote the type of discussion illustrated by Example 2.
There was a problem hiding this comment.
Well, I'm still not sure if I actually conveyed it meaningfully 🙂
Description
Closes #16409.