-
Notifications
You must be signed in to change notification settings - Fork 7
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
INTL-1332: Add import smarts to codemod #241
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
087fe95
to
5e38c29
Compare
2422976
to
fd61b49
Compare
c1cd8d0
to
dc10e48
Compare
b40f695
to
3f9c44b
Compare
@@ -94,6 +113,7 @@ _InsertionLocation _insertionLocationForPackageImport( | |||
final AstNode relativeNode; | |||
final bool insertAfter; | |||
final bool inOwnSection; | |||
bool hasPackageImports = true; |
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 name is not clear, as below you're now setting it to false if there are any relative imports at all. Should maybe be hasRelativeImports?
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 bool
is for enabling true condition where we want imports with the package and if it is not package
then we are setting it to false.
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.
OK. Maybe it should be hasOnlyPackageImports? Or hasAllPackageImports?
@@ -94,6 +113,7 @@ _InsertionLocation _insertionLocationForPackageImport( | |||
final AstNode relativeNode; | |||
final bool insertAfter; | |||
final bool inOwnSection; | |||
bool hasPackageImports = true; |
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.
OK. Maybe it should be hasOnlyPackageImports? Or hasAllPackageImports?
} | ||
|
||
for (final importDirective in imports) { | ||
hasOnlyPackageImports = !imports.any((importDirective) { |
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.
Very close. But these lines are not necessary This is taking something that returns a Uri, turning it into a string, then parsing it so you can ask it for its scheme. Just get the uri and check if it's null and check the scheme.
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.
Initially, we are writing code like this
final uriContent = importDirective.uriContent;
but after merge conflicts resolve there is updating in Analyzer 5 uriContent
is archived and we are using importDirective.uri.stringValue
and if we check scheme with uri like this
hasOnlyPackageImports = !imports.any((importDirective) {
final uri = importDirective.uri;
return uri != null && (uri.scheme != 'package' && uri.scheme != 'dart:');
});
then it's giving error error: The getter 'scheme' isn't defined for the type 'StringLiteral'. (undefined_getter at [over_react_codemod] lib/src/intl_suggestors/intl_importer.dart:158)
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.
Ah, ok. My mistake. I assumed importDirective.uri was returning a Uri, since we needed to convert it to a string. It's a StringLiteral, so this code is good.
} | ||
|
||
for (final importDirective in imports) { | ||
hasOnlyPackageImports = !imports.any((importDirective) { |
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.
Ah, ok. My mistake. I assumed importDirective.uri was returning a Uri, since we needed to convert it to a string. It's a StringLiteral, so this code is good.
QA |
check with wdesk_sdk
|
handle this scenario and update test case but it's getting failed |
@Workiva/release-management-p |
QA +1 CI passes |
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
Problem:
Solution:
_insertionLocationForPackageImport
function is responsible for this.intl
import.decideImportFormat
function.If there are package imports and no relative imports, it selects the package format.
Otherwise, it selects the relative format.