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

PackageManager.getBestPackage: Introduce Version[Range] overloads #2375

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Aug 8, 2022

Continuing on my crusade to have correctly-typed interfaces when it comes to dependencies.

@@ -224,6 +224,7 @@ class PackageManager {

/** Looks up the first package matching the given name.
*/
deprecated("The semantic of this function are unhelpful, prefer `getBestPackage` with `name, Dependency.any` instead")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
deprecated("The semantic of this function are unhelpful, prefer `getBestPackage` with `name, Dependency.any` instead")
deprecated("The semantic of this function are unhelpful, prefer `getBestPackage` with `name, VersionRange.any` instead")

source/dub/dub.d Outdated
logDiagnostic("External sub package %s %s not found.", name, dep.version_);
auto ret = dep.visit!(
(VersionRange rng) => m_dub.m_packageManager.getBestPackage(name, rng),
(any) => assert(0, "Cannot call getPackageRaw with a non-version dependency on subpackages"),
Copy link
Member

Choose a reason for hiding this comment

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

it looks like it'd be possible that this assert(false) branch is called from

getSpecificConfigs -> getPackage -> getPackageRaw

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't be, because otherwise the assert(0) in Dependency.version_ would get triggered.
But I'll look at the CI failure.

Copy link
Member

Choose a reason for hiding this comment

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

the dependency resolution logic is tricky to understand, so I don't really know where to start searching for such a case, but in general these non-version cases are not that well tested, so an edge case is unlikely to trigger in CI here

@Geod24 Geod24 force-pushed the easy-fix-getBestPackage branch from d679b1e to 4888a1e Compare August 9, 2022 01:25
@Geod24 Geod24 force-pushed the easy-fix-getBestPackage branch 2 times, most recently from 76be451 to 2071c5b Compare August 11, 2022 00:06
@Geod24 Geod24 force-pushed the easy-fix-getBestPackage branch from 2071c5b to 346a2d3 Compare August 11, 2022 00:25
@Geod24 Geod24 changed the title PackageManager: Improve getBestPackage and deprecate getFirstPackage PackageManager.getBestPackage: Introduce Version[Range] overloads Aug 11, 2022
@Geod24
Copy link
Member Author

Geod24 commented Aug 11, 2022

@WebFreak001 : Removed deprecation, this should be a no-brainer now (and fix the "TODO").

Copy link
Member

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

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

want to deprecate the Dependency overload or do you see any value in keeping it?

@Geod24
Copy link
Member Author

Geod24 commented Aug 11, 2022

I plan to deprecate it, but the previous iteration didn't pass the CI for unknown reason, so I'll do the deprecation later (it was a side product, not the main target, of the refactoring).

@WebFreak001 WebFreak001 merged commit 01bab5e into dlang:master Aug 11, 2022
@Geod24 Geod24 deleted the easy-fix-getBestPackage branch August 11, 2022 19:27
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