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

Refactor libcnb-package and libcnb-cargo in preparation for test support #628

Closed
wants to merge 1 commit into from

Conversation

colincasey
Copy link
Contributor

The following changes have been made to maximize the sharing of code between libcnb-cargo, libcnb-test, and libcnb-package. This is to prepare for upcoming changes that will allow for testing of local and meta-buildpacks.

The following changes have been made to maximize the sharing of code between `libcnb-cargo`, `libcnb-test`, and `libcnb-package`. This is to prepare for upcoming changes that will allow for testing of local and meta-buildpacks.
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Thank you for splitting out the refactoring from #590 - unfortunately I'm still finding this PR too hard to review as-is, since it's still making a lot of separate changes at once.

Reading the changelog gives a clue that there are actually quite a few parts of this that can be split out, which we can quickly review/merge individually. To speed things up, you could also stack the PRs (ie: set the base branch to the second PR to that of the first, and similar for each PR after that) for any changes that would otherwise result in merge conflicts etc.

@schneems
Copy link
Contributor

In general I agree that features and refactorings should be separate when contributing for review's sake. I also like a tidy(er) git history generally. Also, teasing out this one change into N changes looks like a good bit of time/work. If the main problem is context and the review, one alternative could be to do a pairing session on a review.

I did a pairing session with Colin to walk through changes of the original PR, and that was pretty effective. Maybe you could try that and see what could be merged together and what still needs to be split out?

@colincasey
Copy link
Contributor Author

got it. i'll resist my tendency to go with more of a shotgun-style approach to refactoring and try to slice these changes into smaller parts 😅

@Malax
Copy link
Member

Malax commented Aug 28, 2023

Closing in favour of #657

@Malax Malax closed this Aug 28, 2023
@colincasey colincasey deleted the libcnb_package_refactoring branch August 28, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants