doc: Update some stale internal references and documentation #558
Conversation
|
Hi there! 👋 |
joeyAghion
changed the title
chr-tatu
left a comment
chr-tatu
left a comment
There was a problem hiding this comment.
Looks great! I left a couple of comments.
playbooks/engineer-workflow.md
Outdated
| feature work) against rework (i.e., bug/regression fixes). We currently use this ratio as one measure of code | ||
| quality. **Importantly, this means that consistent commit messages and type accuracy supports meaningful metrics**. | ||
| The ratio of feature to rework can be viewed [here](https://artsy.net/rework-metric). | ||
| While the most common types of commits are `fix`, `feat`, and `chore`, any of `build`, `ci`, `docs`, `perf`, |
There was a problem hiding this comment.
This info feels redundant. These are all part of:
Artsy prefers Conventional Commits
| to read the [spec](https://www.conventionalcommits.org/en/v1.0.0/#specification) for more detail but, in general, | ||
| all commits should match the following format | ||
| ([see examples](https://www.conventionalcommits.org/en/v1.0.0/#examples)): | ||
| Artsy prefers [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/#summary) (see |
There was a problem hiding this comment.
There's nothing anymoreafter "(see", right?
There was a problem hiding this comment.
There is--automatically wrapped to the next line. (I dislike this auto-formatting but 🤷 ).
There was a problem hiding this comment.
Shall we remove this auto-formating? I also don't like it. It makes it harder to read PRs, since it shows changes where there is none.
| We encourage you to assign your completed PRs to one of your reviewers in order to maintain momentum and to | ||
| distribute responsibilities and understanding. However self-assigning is permissible when an author wants to retain | ||
| control over merging, sequencing or follow-ups related to the PR. | ||
| **Assign** a single team member to the PR. This is the person who is responsible for ultimately merging. It's |
There was a problem hiding this comment.
In the teams I managed, I always challenged this approach. I encouraged self-assign. This was the responsibility stays with the same person for both creating and merging the PR once they have gather and acted on enough feedback. My take is that pushing a PR to someone else, dissipates the responsibility.
There was a problem hiding this comment.
I say "share" where you say "dissipate." Ultimately the maintenance or follow-ups will need to be owned by the team and not an individual, so I like teammates to start feeling invested--having "skin in the game"--at this stage. I do say "self-assigning is fine if an author wants to retain control over merging..." below, but often that control (and additional hand-off) shouldn't be necessary if the communication and safeguards and tests are working.
| themes: | ||
|
|
||
| **Cost** | ||
| - <details><summary>ROI</summary> |
There was a problem hiding this comment.
So nice to see these tags gone!
As discussed among managers, let's refresh these docs before they confuse candidates or onboardees.
main) and the valid ways to achieve that.Some formatting churn was automatic. 😐