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

Prune non-importable audits. #583

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

afranchuk
Copy link
Collaborator

This addresses part of #580.

@afranchuk afranchuk requested a review from mystor December 7, 2023 21:37
src/cli.rs Outdated Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
@@ -2744,7 +2756,9 @@ fn resolve_package_required_entries(
DeltaEdgeOrigin::Unpublished { unpublished_index } => {
add_entry(RequiredEntry::Unpublished { unpublished_index })
}
DeltaEdgeOrigin::StoredLocalAudit { .. } => {}
DeltaEdgeOrigin::StoredLocalAudit { audit_index, .. } => {
add_entry(RequiredEntry::LocalAudit { audit_index })
Copy link
Collaborator

Choose a reason for hiding this comment

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

We only need to add an entry here if the type is non-importable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While that's technically true (since that's the only thing that would be a candidate for pruning in the first place), I thought it'd be more intuitive to track all local audits that are used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't track all of them originally because that information exists in DeltaEdgeOrigin and RequiredEntry only exists for pruning. Doesn't matter a ton though.

src/resolver.rs Outdated Show resolved Hide resolved
src/resolver.rs Show resolved Hide resolved
src/resolver.rs Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
@mystor mystor merged commit 5cfaa7f into mozilla:main Dec 8, 2023
8 checks passed
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