-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fix/delegate __lt__
to PackageVersionLike
#448
Fix/delegate __lt__
to PackageVersionLike
#448
Conversation
v3 = PackageVersion("latest") | ||
v3 = PackageVersion("0.12.0") | ||
assert v1 < v2 < v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this won't work otherwise, test was not proper since relied on string comparison ("0" < "l" == True
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: we do not delegate to semver.VersionInfo
to maintain the ability to use "latest"
and "any"
, however we cannot compare PackageVersion("1.0.0") < PackageVersion("latest")
, which will raise ValueError: latest is not valid SemVer string
due to the deflection. See Version.__lt__
and Version.compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this change does not make sense at all, the latest
version should always be the superior when comparing to other versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Au contraire mon ami.
- This change would have made the original implementation fail.
- Comparing "latest" should not always return
True
, in case latest is equal to current version being compared to. In this sense "latest" is a property. - We also have this "any" which poses another issue.
Hence, additional tests are required to cover these cases, for which I can stack a PR later this week. Prior to that, however, we need to decide how to deal with these. As I stated in my base PR (#428) we should reconsider whether we want to maintain "any" and "latest".
put a #428 (comment) in base PR to address later so we can merge #445, #447 and #448 into it.
sorted_expected = list(map(str, sorted(self.expected_dependencies))) | ||
sorted_missing = list(map(str, sorted(self.missing_dependencies))) | ||
expected = list(map(str, self.expected_dependencies)) | ||
missing = list(map(str, self.missing_dependencies)) | ||
print("=" * 50) | ||
print(f"Package {self.configuration_file}:") | ||
print(f"Expected: {pprint.pformat(sorted_expected)}") | ||
print(f"Missing: {pprint.pformat(sorted_missing)}") | ||
print(f"Expected: {pprint.pformat(expected)}") | ||
print(f"Missing: {pprint.pformat(missing)}") | ||
print("=" * 50) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sortable when author and name differ
flat_tree.append( | ||
sorted( | ||
{package for package in tree_level if package not in dirty_packages} | ||
) | ||
) | ||
flat_tree.append([]) | ||
for package in tree_level: | ||
if package not in dirty_packages and package not in flat_tree[-1]: | ||
flat_tree[-1].append(package) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sortable, this way we obtain a unique set that maintains a deterministic ordering
__lt__
to PackageVersionLike
__lt__
to PackageVersionLike
__lt__
to PackageVersionLike
__lt__
to PackageVersionLike
__lt__
to PackageVersionLike
__lt__
to PackageVersionLike
2ec503d
to
4784ac1
Compare
f6889df
to
779c951
Compare
Codecov ReportBase: 97.31% // Head: 97.30% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## fix/data_types__lt__ #448 +/- ##
========================================================
- Coverage 97.31% 97.30% -0.01%
========================================================
Files 221 221
Lines 19543 19548 +5
========================================================
+ Hits 19018 19022 +4
- Misses 525 526 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
queue.extendleft(sorted(roots)) # type: ignore | ||
queue.extendleft(sorted(roots, key=str)) # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this guarantees the ordering of root nodes is deterministic, and hence the returned List[T]
of find_topological_order
will be as well.
__lt__
to PackageVersionLike
__lt__
to PackageVersionLike
Proposed changes
Delegate comparison operations to
PackageVersionLike._version
in custom__lt__
methods on custom data types:PackageVersion
,PublicId
andPackageId
.Fixes
If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce to agents-aea?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply.develop
branch (left side). Also you should start your branch off ourdevelop
.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...