[windows] Refactor to optimize vswhere queries #40197
Conversation
768be63 to
ec8b893
Compare
Codecov Report
@@ Coverage Diff @@
## master #40197 +/- ##
==========================================
+ Coverage 58.44% 58.8% +0.35%
==========================================
Files 192 192
Lines 18627 18636 +9
==========================================
+ Hits 10886 10958 +72
+ Misses 7741 7678 -63
Continue to review full report at Codecov.
|
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Thanks! Sorry for writing this technical debt into the implementation in the first place.
There was a problem hiding this comment.
The getters should never return null, so you can simplify these checks to just the isNotEmpty part.
There was a problem hiding this comment.
Optional: you could simplify the call sites using this by having it return an empty map instead of null; then they don't need to do null checks.
There was a problem hiding this comment.
As above, you only need the second part.
There was a problem hiding this comment.
Let's add ", or {} if there is no such installation" to the end of the first sentence so the return contract is explicit up front.
The second part should be a new paragraph per Dart style.
Same for the other two *Details getters.
There was a problem hiding this comment.
This is essentially duplicating the note a few lines above; remove one or the other copy.
There was a problem hiding this comment.
Optional: I found this re-assignment construction (where the usable version variable is set to a value that might not actually be usable, then unset if needed) a bit hard to follow; I think a local variable for the lookup would make it clearer:
if (_cachedUsableVisualStudioDetails == null) {
Map<String, dynamic> visualStudioWithComponentsDetails = [...];
visualStudioWithComponentsDetails ??= [...];
if (visualStudioWithComponentsDetails != null) {
if (installationHasIssues(visualStudioWithComponentsDetails) {
_cachedUsable... = visualStudioWithCompontesDetails;
} else {
_cachedAny... = visualStudioWithCompontesDetails;
}
}
_cachedUsableVisualStudioDetails ??= <String, dynamic>{};
}
return _cachedUsableVisualStudioDetails;
ec8b893 to
c9c0296
Compare
eec098d to
f738cfe
Compare
|
This is good for review, PTAL! |
| Map<String, dynamic> _cachedUsableVisualStudioDetails; | ||
| Map<String, dynamic> get _usableVisualStudioDetails { | ||
| _cachedUsableVisualStudioDetails ??= | ||
| Map<String, dynamic> visualStudioDetails = |
There was a problem hiding this comment.
You're running this on every call, even if a result was already cached. This should all be inside an if-null check (I believe I had that in my suggested code in the other PR).
There was a problem hiding this comment.
🤦♂ Added it as a check for != null and early return to avoid wrapping all the logic inside an if
Description
visual_studio.dart is not keeping track whether it has queried vswhere and didn't find an installation, of it hasn't performed any queries at all. This may cause unnecessary queries on the former case. This PR assigns a sentinel value of an empty map to the cached installation details to avoid repeating the queries.
Related Issues
#40196
Tests
I added the following tests:
This is a refactor so the behavior should continue as before.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?