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

Compel users to release packages with breaking changes alongside their dependents #101

Merged
merged 11 commits into from
Oct 17, 2023

Conversation

cryptodev-2s
Copy link
Contributor

This PR Compel users to release packages with breaking changes alongside their dependents.

Issue: 90

@cryptodev-2s cryptodev-2s requested a review from a team as a code owner September 28, 2023 13:09
@cryptodev-2s cryptodev-2s force-pushed the feature/compel-users-to-release-dependents branch from eff572d to 33d8a21 Compare September 28, 2023 13:10
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks great! Left a few suggestions, the most important of which is ensuring that packages set to null still produce this error

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Sorry it took me so long to get to this PR. I was unable to get to the end of this PR today, but would definitely like to take a closer look Monday.

@cryptodev-2s cryptodev-2s requested a review from mcmire October 6, 2023 15:21
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

This is good! A lot of the comments here have to do with simplified ways of doing the same thing or tweaks to wording, but there are some test cases I think we will want to add as well.

@cryptodev-2s cryptodev-2s requested a review from mcmire October 11, 2023 20:23
mcmire
mcmire previously approved these changes Oct 17, 2023
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

I have a few more suggestions for making this code a bit easier to read, but I know I've also suggested refactoring this code, so considering you've been waiting on this PR for a while, that's something we can put off. The logic at least seems correct here, so feel free to merge!

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks good! Again sorry for the long wait on this.

@cryptodev-2s cryptodev-2s merged commit 0f474e9 into main Oct 17, 2023
@cryptodev-2s cryptodev-2s deleted the feature/compel-users-to-release-dependents branch October 17, 2023 20:52
cryptodev-2s added a commit to MetaMask/core that referenced this pull request Dec 7, 2023
## Explanation

This PR updates the release process to take in considerations recent
changes on create-release-branch.
- Compel users to release packages with breaking changes alongside their
dependents [MetaMask/create-release-branch#101]
- Compel users to release new versions of dependencies alongside their
dependents [MetaMask/create-release-branch#102]
- Reorder workflow to update changelogs first
[MetaMask/create-release-branch#109]

## References

- Closes #1741

## Changelog

N/A

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
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