fix: incorrect new dependency logic #304
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change fixes a problem where _any_ change in a lockfile results in the finding that _all_ dependencies from the lockfile are new. This was introduced in #282, when the `PackageDescriptor` dataclass added the `lockfile` attribute. The issue is that the `Lockfile` class uses the `PackageDescriptor` dataclass to make comparisons and compute unique sets of dependencies. The `get_previous_lockfile_packages` function works by writing the previous lockfile object to a temporary file and then parsing that file with the `phylum parse` command. The temporary file will never have the same name as the current lockfile and so the `lockfile` attribute will be different for _all_ dependencies in the given lockfile. Making this change ensures the `lockfile` attribute of the dataclass is not included when generating equality and comparison methods. There is not a problem with this exclusion because the `Lockfile` class still only operates on one lockfile at a time and keeps track of the lockfile's path separately, in the `path` property.
maxrake
requested review from
cd-work and
kylewillmon
and removed request for
cd-work
kylewillmon
approved these changes
Sep 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change fixes a problem where any change in a lockfile results in the finding that all dependencies from the lockfile are new. This was introduced in #282, when the
PackageDescriptordataclass added thelockfileattribute.The issue is that the
Lockfileclass uses thePackageDescriptordataclass to make comparisons and compute unique sets of dependencies. Theget_previous_lockfile_packagesfunction works by writing the previous lockfile object to a temporary file and then parsing that file with thephylum parsecommand. The temporary file will never have the same name as the current lockfile and so thelockfileattribute will be different for all dependencies in the given lockfile.Making this change ensures the
lockfileattribute of the dataclass is not included when generating equality and comparison methods. There is not a problem with this exclusion because theLockfileclass still only operates on one lockfile at a time and keeps track of the lockfile's path separately, in thepathproperty.Testing Results
Example of the behavior BEFORE the changes in this PR (a known bad dependency was added)
Example of the behavior AFTER the changes in this PR
Notice the difference in "unique newly added dependencies" going from 85 to 1.