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

Simplify Resolution type #5301

Merged
merged 2 commits into from
Jul 22, 2024
Merged

Simplify Resolution type #5301

merged 2 commits into from
Jul 22, 2024

Conversation

konstin
Copy link
Member

@konstin konstin commented Jul 22, 2024

Looking at how to merge identical forks, i found that the Resolution can be simplified by treating it as a nodes and edges store (plus pins, they are separate since they are per name-version, not per (virtual-)package-version). This should also make #5294 more apparent, which i didn't touch here.

I additionally added some doc comments to the Resolution types.

@konstin konstin added the internal A refactor or improvement that is not user-facing label Jul 22, 2024
@konstin konstin requested a review from BurntSushi July 22, 2024 18:03
/// The resolution from one or more forks including the virtual packages and the edges between them.
///
/// Each package can have multiple versions and each edge between two packages can have multiple
/// version specifiers to support diverging versions and requirements in different forks.
#[derive(Debug, Default)]
pub(crate) struct Resolution {
pub(crate) packages: FxHashMap<ResolutionPackage, FxHashSet<Version>>,
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename this field to nodes?

Copy link
Member

Choose a reason for hiding this comment

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

E.g. "nodes and edges" rather than "packages and edges"

Base automatically changed from konsti/slim-down-solve to main July 22, 2024 18:52
@konstin konstin force-pushed the konsti/save-forks-in-lockfile branch from f4ba2d3 to 0a08734 Compare July 22, 2024 18:54
@charliermarsh charliermarsh merged commit 0a95b7a into main Jul 22, 2024
54 checks passed
@charliermarsh charliermarsh deleted the konsti/save-forks-in-lockfile branch July 22, 2024 19:10
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Interesting, yeah, I think this is simpler. I wonder if our access patterns changed since I originally wrote this type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal A refactor or improvement that is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants