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

Specify that PR can combine multiple changes, especially for dependencies upgrade #164

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

mchv
Copy link
Member

@mchv mchv commented Apr 11, 2024

What is being recommended

The current emphasis in our recommendation regarding PR is too strong on their size, leading to creation of PRs smaller necessaries which is not very efficient.

The associated change reduce that emphasis, explicitly mentioning that some dependencies upgrade to be grouped avoiding typical compatibility issues and bugs, and preventing people to focus on minimising number changes rather than delivering them.

What's the context?

Note

The recommendations in this repository are intended to share engineering best practices for all of Product & Engineering (P&E) in the open.

Some considerations:

  • Should this really be an ADR in another repository?
  • Have you sought review widely enough (Developer Experience, Heads of Engineering)?
  • If it is security related, should you consult with Information Security?

Copy link
Member

@kelvin-chappell kelvin-chappell left a comment

Choose a reason for hiding this comment

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

👍

pull-requests.md Outdated Show resolved Hide resolved
pull-requests.md Outdated Show resolved Hide resolved
mchv and others added 2 commits April 15, 2024 15:04
Co-authored-by: Kelvin Chappell <[email protected]>
Co-authored-by: Kelvin Chappell <[email protected]>
@mchv mchv merged commit 18441d3 into main Apr 17, 2024
1 check passed
@mchv mchv deleted the mc-improve-PR-recommendations branch April 17, 2024 15:13
@mxdvl
Copy link
Contributor

mxdvl commented Apr 22, 2024

I wonder if this applies to the NPM ecosystem?

Due to the number of different package managers, lockfile formats, prominent packages such as TypeScript not following SemVer, prominent packages such as Storybook not following peer dependencies declarations correctly and most of our internal project following our own recommendations of pinning dependencies, I fear this recommendation will cause more confusion and delays than speed up delivery and understanding1.

Footnotes

  1. I can provide links to all the sources to these claims in a private chat as I do not want to bring any public attention to the specific shortcomings above on a public forum.

@mchv
Copy link
Member Author

mchv commented Apr 23, 2024

@mxdvl I think you should raise a PR with improvements on the wording/recommendation on the NPM ecosystem based on your experience and the issues you mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants