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

Clean up the UnpublishedOperation struct #3174

Merged
merged 4 commits into from
Mar 3, 2024
Merged

Conversation

emesterhazy
Copy link
Contributor

This PR makes a few minor refactoring changes to UnpublishedOperation. The behavior is left unchanged.

Happy to split this into separate PRs if that's easier, but these three commits together complete the cleanup.

lib/src/transaction.rs Outdated Show resolved Hide resolved
lib/src/transaction.rs Show resolved Hide resolved
@emesterhazy emesterhazy force-pushed the push-vqzvxzsozyol branch 3 times, most recently from 1bb1732 to d9b0317 Compare March 2, 2024 19:52
@emesterhazy emesterhazy requested review from yuja and removed request for martinvonz March 2, 2024 19:53
The custom Drop impl prevents us from moving members of UnpublishedOperation,
and is the reason why `NewRepoData` is wrapped in an `Option`. We don't use
custom Drop functions like this for debugging elsewhere in the codebase, and in
some ways #[must_use] provides better protection since it will typically cause
a compiler error if the UnpublishedOperation isn't used.
The Option is unnecessary now since `UnpublishedOperation` doesn't implement
the Drop trait (the `MustClose` member implements it instead).
`NewRepoData` is just a container that holds data used to construct a
`ReadonlyRepo`. The `ReaonlyRepo` is always constructed before the
`UnpublishedOperation` is dropped, so we can simply construct the
`ReadonlyRepo` upfront and delete the `NewRepoData` type.
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

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

nice!

lib/src/transaction.rs Show resolved Hide resolved
The only thing we need from the `RepoLoader` is the `OpHeadsStore`, so we can
extract it in UnpublishedOperation::new instead of keeping the entire
`RepoLoader` around.
@emesterhazy emesterhazy merged commit ff4a5aa into main Mar 3, 2024
16 checks passed
@emesterhazy emesterhazy deleted the push-vqzvxzsozyol branch March 3, 2024 04:08
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.

2 participants