Skip to content
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

Merged
merged 65 commits into from
Nov 1, 2023
Merged

INTL-1332: Add import smarts to codemod #241

merged 65 commits into from
Nov 1, 2023

Conversation

chetansinghchoudhary-wk
Copy link
Contributor

Problem:

  • Add the ability for the codemod to decide how the intl class import statement will be formatted, ie. package format vs. relative format. It should behave something like this:
    • If the file has only package imports, go with a package format
    • If the file has only relative imports, go with a relative format
    • If the file has a mix of both, go with the package format
    • If the file has no imports, go with the package format

Solution:

  • Determine where to insert the import statement. The _insertionLocationForPackageImport function is responsible for this.
  • This function inspects the existing import directives in the file and calculates the appropriate insertion point for the intl import.
  • Tracked whether the file has package or relative imports by iterating through the existing import directives.
  • After determining the insertion location and identifying existing imports, decide on the import format. This is done in the decideImportFormat function.
    If there are package imports and no relative imports, it selects the package format.
    Otherwise, it selects the relative format.

@aviary3-wk
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@chetansinghchoudhary-wk chetansinghchoudhary-wk marked this pull request as ready for review September 13, 2023 12:49
lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
test/intl_suggestors/intl_importer_test.dart Outdated Show resolved Hide resolved
lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
lib/src/intl_suggestors/intl_importer.dart Show resolved Hide resolved
@@ -94,6 +113,7 @@ _InsertionLocation _insertionLocationForPackageImport(
final AstNode relativeNode;
final bool insertAfter;
final bool inOwnSection;
bool hasPackageImports = true;
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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?

test/intl_suggestors/intl_importer_test.dart Outdated Show resolved Hide resolved
lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
@@ -94,6 +113,7 @@ _InsertionLocation _insertionLocationForPackageImport(
final AstNode relativeNode;
final bool insertAfter;
final bool inOwnSection;
bool hasPackageImports = true;
Copy link
Contributor

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?

lib/src/intl_suggestors/intl_importer.dart Outdated Show resolved Hide resolved
}

for (final importDirective in imports) {
hasOnlyPackageImports = !imports.any((importDirective) {
Copy link
Contributor

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.

Copy link
Contributor

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)

Copy link
Contributor

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.

alanknight-wk
alanknight-wk previously approved these changes Oct 20, 2023
}

for (final importDirective in imports) {
hasOnlyPackageImports = !imports.any((importDirective) {
Copy link
Contributor

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.

@alanknight-wk
Copy link
Contributor

QA
The code seems reasonable now, but I ran it for QA on a few repos and it didn't do what I expected.
graph_app - no changes, but they have the codemod in their CI now, so I didn't expect any, and nothing broke.
doc_plat_client - only one change, but it was to a file with no other imports from that package, and it made it a relative import, which doesn't seem right as the default
wdesk_sdk - The oddest one. In lib/src/analytics/shell_analytics.dart there was only one previous import, to another package, it added the intl import as package: But in lib/src/experiences/configs/wdata_experience_config.dart, which did have package: imports in the current package it added it as relative.

@akshaybhardwaj-wk
Copy link
Contributor

lib/src/experiences/configs/wdata_experience_config.dart

check with wdesk_sdk

  1. lib/src/analytics/shell_analytics.dart In this file I am getting package import
  2. lib/src/experiences/configs/wdata_experience_config.dart In this I am getting relative because this file has both dart: and package import we have to handle this case and also create a needed test case .

@akshaybhardwaj-wk
Copy link
Contributor

lib/src/experiences/configs/wdata_experience_config.dart

check with wdesk_sdk

  1. lib/src/analytics/shell_analytics.dart In this file I am getting package import
  2. lib/src/experiences/configs/wdata_experience_config.dart In this I am getting relative because this file has both dart: and package import we have to handle this case and also create a needed test case .

handle this scenario and update test case but it's getting failed

@alanknight-wk
Copy link
Contributor

@Workiva/release-management-p

@alanknight-wk
Copy link
Contributor

QA +1 CI passes

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from RM

@rmconsole7-wk rmconsole7-wk merged commit dde6403 into master Nov 1, 2023
8 checks passed
@rmconsole7-wk rmconsole7-wk deleted the INTL-1332 branch November 1, 2023 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants