-
Notifications
You must be signed in to change notification settings - Fork 2
FEA-1596: Fixed dart install location issue #20
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
Conversation
Merge Requirements Met ✅Request Rosie to automerge this pull request by including @Workiva/release-management-p in a comment. General InformationTicket(s): Code Review(s): #20
Reviewers: evanweible-wf, matthewnitschke-wk Additional InformationWatchlist Notifications: None
Note: This is a shortened report. Click here to view Rosie's full evaluation. |
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
lib/src/symbol.dart
Outdated
.substring(sourcePath.indexOf('dart-sdk/lib/') + 'dart-sdk/'.length); | ||
|
||
if (element.enclosingElement?.source?.uri != null) { | ||
filePath = element.enclosingElement!.source!.uri.toString(); |
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 is pulled directly from how dart resolves the isInSdk
check: https://sourcegraph.com/github.com/dart-lang/sdk/-/blob/pkg/analyzer/lib/src/dart/element/element.dart?L4196
Instead of resolving a path to the root, this will return dart:<package>/path/to/file.dart
. While this is technically changing the output of the scip, and and a breaking change, since we dont index the sdk and rely on analysis between references/definitions, this should be ok
QA +1 Verified scip snapshots are still ok, minor changes to symbols, but since we dont index dart/sdk, everything should be ok |
🚀 @Workiva/release-management-p 🚢 |
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.
+1 from RM
FEA-1596
Previously there were some assumptions in folder paths for resolving references to dart sdk packages. Essentially, we were assuming
'dart-sdk/lib/'
would exist in the path to the dart core, which is not a guarantee.Instead of relying on this, we use the uri for the element's enclosing element:
element.enclosingElement.source.uri
, which correlates to the path for the element's file in the dart sdk. Ex:dart:core/path/to/whatever.dart
, we use this full path for the scip symbol, and use it substringed by/
('dart:core/path/to/whatever.dart'.split('/').first
) for retrieving the package nameThese changes result in a more consistent calculation of dart sdk symbols, and will work regardless of how dart was installed on the system