-
-
Notifications
You must be signed in to change notification settings - Fork 228
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: Make lookup lazy again #2923
Conversation
✅ PR OK, no changes in deprecations or warnings Total deprecations: 0 Total warnings: 0 Build statistics: statistics (-before, +after)
executable size=5259464 bin/dub
rough build time=62s Full build output
|
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.
Looks like this shouldn't interfere with anything. I'd only suggest to replace the two bool
s by a three-stage InitializationState
enum. As a bonus, the conditional refresh
and refreshLocal
calls could then be replaced by call to an initialize(InitializationState)
to make it a little less repetitive and semantically clearer.
I'd love to get rid of refresh entirely, but the value is in making |
source/dub/packagemanager.d
Outdated
bool m_initialized; | ||
enum InitializationState { | ||
/// No `refresh*` function has been called | ||
None, |
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.
Maybe a nit pick, but both style guides (D/vibe.d) want this to be camelCase
.
edit: I didn't realize this is protected
and PackageManager
is now meant to be derived from. In this case we need to be a bit more careful here to not introduce too much surface for later breaking changes. Maybe this should just be in a private
block instead.
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.
Always found it pretty ugly, but it's the D style. Fixed.
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.
I didn't realize this is
protected
andPackageManager
is now meant to be derived from.
Note that the main use case for this is currently for unittesting.
We want to avoid loading the whole cache whenever lookup is called, as most invocations of dub should result in a call to lookup, since dependency resolution is much rarer than building with an existing dub.selections.json.
We want to avoid loading the whole cache whenever lookup is called, as most invocations of dub should result in a call to lookup, since dependency resolution is much rarer than building with an existing dub.selections.json.