[XL] Change in dart/sdk[main]: [DAS] Adds completion option for some shadowed names
FMorschel (Gerrit)
FMorschel has uploaded the change for review![]( "Open in Gerrit")
Commit message
[DAS] Adds completion option for some shadowed names
Change information
- M pkg/analysis_server/lib/src/services/completion/dart/candidate_suggestion.dart
- M pkg/analysis_server/lib/src/services/completion/dart/declaration_helper.dart
- M pkg/analysis_server/lib/src/services/completion/dart/not_imported_completion_pass.dart
- M pkg/analysis_server/lib/src/services/completion/dart/visibility_tracker.dart
- M pkg/analysis_server/test/client/completion_driver_test.dart
- M pkg/analysis_server/test/lsp/completion_dart_test.dart
- A pkg/analysis_server/test/services/completion/dart/visibility/scope_shadow_test.dart
- M pkg/analysis_server/test/services/completion/dart/visibility/test_all.dart
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
| Commit-Queue | +1 |
2 comments
'$_enclosingPrefix$qualifier${element.displayName}';It's an error if both of these are non-empty strings (both here and on line 1136).
Consider adding an assert to ensure that `withEnclosingName` is never `true` when `qualifier` is non-empty. Or consider having two constructors, one that takes `withEnclosingName` and one that takes `qualifier`.
this?.callWhile technically correct, this is confusing. For the test we can tell the difference because you made them different kinds (thank you!). But for users I'm a bit concerned that it might be hard to tell them apart. I don't know what to do about it, but wanted to mention it.
Related details
- FMorschel
- Keerti Parthasarathy
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
FMorschel (Gerrit)
FMorschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by FMorschel
| Auto-Submit | +1 |
2 comments
'$_enclosingPrefix$qualifier${element.displayName}';It's an error if both of these are non-empty strings (both here and on line 1136).
Consider adding an assert to ensure that `withEnclosingName` is never `true` when `qualifier` is non-empty. Or consider having two constructors, one that takes `withEnclosingName` and one that takes `qualifier`.
I've done a small refactor around the constructors for members here. So now they all have the same logic.
this?.callWhile technically correct, this is confusing. For the test we can tell the difference because you made them different kinds (thank you!). But for users I'm a bit concerned that it might be hard to tell them apart. I don't know what to do about it, but wanted to mention it.
Yes, I also thought so. Although I don't suspect this is really common, I just wanted to make sure we were covering this edge case correctly. I'm fine with removing the `this?.` option, but I think it would come up in so few cases that I think it's fine for us to tell users this is how their code is overlapping and could probably use a rename somewhere 😅.
Related details
- Brian Wilkerson
- Keerti Parthasarathy
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted![]( "Open in Gerrit")
- FMorschel
- Keerti Parthasarathy
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |