Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Reference commits in changelog when no PR #44115

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Oct 15, 2024

I occasionally, or quite frequently, push commits on the master branch of Material UI, Base UI, MUI X, Pigment CSS, etc. anytime I see a change that:

  • a. Has no knowledge-sharing needs, not much room to delegate it, or at least it doesn't feel like the added latency in the PR review time. I don't want to overwhelm the PR review capacity of the team, when it feels like it would overload it.
  • b. Is trivial, no much value is having a second pair of eyes.
  • c. Is safe, no much value in running the CI against it.

(I get this balance wrong sometimes, I should likely be more moderate with this). In any case, there are use cases, I think ones that could be open to more team members.

Here is the value of this change, better DX for developers in the GitHub release page:

 - [code-infra] Link to production app for bundle size (#44076) @oliviertassinari
 - [core] Remove [website] from changelog (#44069) @oliviertassinari
-- [core] Apply #44052 to the latest release as well @oliviertassinari
-- [docs] Fix 404 link to Next.js @oliviertassinari
-- [examples] Avoid git diff when playing with examples @oliviertassinari
+- [core] Apply #44052 to the latest release as well (82a8253) @oliviertassinari
+- [docs] Fix 404 link to Next.js (a25a365) @oliviertassinari
+- [examples] Avoid git diff when playing with examples (bfe28ad) @oliviertassinari
 - [website] Fix outdated references to XGrid (#44046) @oliviertassinari

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Oct 15, 2024
@mui-bot
Copy link

mui-bot commented Oct 15, 2024

Netlify deploy preview

https://deploy-preview-44115--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 0e80eb2

Signed-off-by: Olivier Tassinari <[email protected]>
Signed-off-by: Olivier Tassinari <[email protected]>
Signed-off-by: Olivier Tassinari <[email protected]>
Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@mnajdova
Copy link
Member

mnajdova commented Oct 16, 2024

I am ok with the change, but honestly I would advocate against this in general.

a. Has no knowledge-sharing needs, not much room to delegate it, or at least it doesn't feel like the added latency in the PR review time. I don't want to overwhelm the PR review capacity of the team, when it feels like it would overload it.

If the PR is trivial the PR review time would be very short, so I don't think we should worry about this. On the other hand if the PR is not trivial, we likely want a review anyway.

b. Is trivial, no much value is having a second pair of eyes.

Yes, but if people see you fixing similar stuff a few times, they may catch up on the idea that this is important and keep an eye for it themselves. So yes, it's trivial or it wouldn't teach people a lot, but it may bring awareness of some issues that people tend to ignore.

c. Is safe, no much value in running the CI against it.

This is valuable if you need to do a quick release/publish the docs, otherwise, I don't see the value.

@aarongarciah
Copy link
Member

Agree with @mnajdova's points. Even the simplest changes can leave master in a bad state e.g. a documentation change that introduces a Vale lint error.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 16, 2024

If the PR is trivial the PR review time would be very short, so I don't think we should worry about this. On the other hand if the PR is not trivial, we likely want a review anyway.

@mnajdova I Agree, the problem is that it: 1. usually takes days. context shifting cost 2. PR overhead (I could look into CLI automation)

Maybe the solution is to go back to the PR review timeline that we used to use during v1 to v4 era with Sebastian: 3 days. If no reviews and you have a positive review balance (more reviews left than PR opened), you can merge after 3 days. e.g. I should merge mui/pigment-css#277 before the end of the week (I hesitated to make a direct commit, but wanted to benefit from the opposite of point a.)

I would also do batch PRs at that time, e.g. #28381 has 12 small ones. It had a few cases where I was frustrated that it made git blame harder. But I was doing so many of them, x4+ more than today, that it was probably the best compromise.

Yes, but if people see you fixing similar stuff a few times, they may catch up on the idea that this is important and keep an eye for it themselves. So yes, it's trivial or it wouldn't teach people a lot, but it may bring awareness of some issues that people tend to ignore.

This sounds about a. I would ideally not work on those but, if the problem, in the opportunity cost referential, look like nice value / low effort, and especially if it has been here for a while. I might as well go for it. I think that seeing (painful enough to be seen) something unsolved for a long time (team spread thin) that can be fixed quickly (<15 minutes) feels like a signal enough that I should prioritize it.

This is valuable if you need to do a quick release/publish the docs, otherwise, I don't see the value.

With infra items, we sometimes need to push small changes to Base UI, Pigment CSS, MUI X, MUI Private, MUI Public, and Material UI. 6 PRs. It's tough.

I will continue to try to refrain myself on making those direct commits 👍

Even the simplest changes can leave master in a bad state e.g. a documentation change that introduces a Vale lint error.

@aarongarciah Yeah, this is definitely a case of overreach for those commits when it breaks the CI.

@oliviertassinari oliviertassinari merged commit 90b890a into mui:master Oct 16, 2024
19 checks passed
@oliviertassinari oliviertassinari deleted the improve-release-changelog-script branch October 16, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants