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-1608 : Activate _over_react_codemod executables separately #246

Merged
merged 16 commits into from
Nov 9, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions bin/sort_intl.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2023 Workiva Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

export 'package:over_react_codemod/src/executables/sort_intl.dart';
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
329 changes: 329 additions & 0 deletions lib/src/executables/sort_intl.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,329 @@
// Copyright 2023 Workiva Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import 'dart:io';
import 'package:analyzer/dart/analysis/utilities.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:args/args.dart';
import 'package:codemod/codemod.dart';
import 'package:file/file.dart';
import 'package:file/local.dart';
import 'package:glob/glob.dart';
import 'package:glob/list_local_fs.dart';
import 'package:logging/logging.dart';
import 'package:over_react_codemod/src/intl_suggestors/intl_messages.dart';
import 'package:over_react_codemod/src/util/package_util.dart';
import 'package:path/path.dart' as p;

import '../util.dart';
import 'intl_message_migration.dart';

final _log = Logger('orcm.intl_message_migration');

const _verboseFlag = 'verbose';

const _noMigrate = 'no-migrate';
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved

const _allCodemodFlags = {
_verboseFlag,
};

final FileSystem fs = const LocalFileSystem();

final parser = ArgParser()
..addFlag(
'help',
abbr: 'h',
negatable: false,
help: 'Prints this help output.',
)
..addFlag(
'verbose',
abbr: 'v',
negatable: false,
help: 'Outputs all logging to stdout/stderr.',
)
..addFlag(_noMigrate,
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
negatable: false,
defaultsTo: false,
help:
'Does not run any migrators, overriding any --migrate flags. Can still be used with --prune-unused, and '
'will force the messages file to be sorted and rewritten');

late ArgResults parsedArgs;

void main(List<String> args) async {
parsedArgs = parser.parse(args);
if (parsedArgs['help'] as bool) {
printUsage();
return;
}

// It's easier to only pass through codemod flags than it is to try to strip
// out our custom flags/options (since they can take several forms due to
// abbreviations and the different syntaxes for providing an option value,
// especially the two-arg `--option value` syntax).
//
// An alternative would be to use `--` and `arguments.rest` to pass along codemod
// args, but that's not as convenient to the user and makes showing help a bit more complicated.

final codemodArgs = _allCodemodFlags
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
.where((name) => parsedArgs[name] as bool)
.map((name) => '--$name')
.toList();

// codemod sets up a global logging handler that forwards to the console, and
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
// we want that set up before we do other non-codemod things that might log.
//
// We could set up logging too, but we can't disable codemod's log handler,
// so we'd have to disable our own logging before calling into codemod.
// While hackier, this is easier.
// TODO each time we call runInteractiveCodemod, all subsequent logs are forwarded to the console an extra time. Update codemod package to prevent this (maybe a flag to disable automatic global logging?)
exitCode = await runInteractiveCodemod(
[],
(_) async* {},
args: codemodArgs,
additionalHelpOutput: parser.usage,
);
if (exitCode != 0) return;
print('^ Ignore the "codemod found no files" warning above for now.');

// If we have specified paths on the command line, limit our processing to
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two cases we have to consider for this. The first is when we have a normal repo with a single package that has a file lib/src/intl/packagename_intl.dart in it. For example, wdesk. This executable works for that. But the reason it works is that this code finds every Dart file underneath /lib and finds a single package root which is the root of the repo. But in that case, finding the package root is not really necessary, because it's the known root of the repo.

The more complicated case is when a repo has multiple subpackages, for example ml_ui_client. In that case finding the package roots would be useful, but this doesn't do it properly because it only looks for files underneath /lib. There is no top-level lib directory in ml_ui_client, so it doesn't find anything and throws an exception. Could not find package root for file /Users/alanknight/dart/ml_ui_client/lib

// those, and make sure they're absolute.
Copy link
Contributor

Choose a reason for hiding this comment

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

So there are two directions this could go.

  1. Don't find all the Dart files. Find all the _intl.dart files under the root directory, but exclude test. Then I think this code would more or less work.
  2. Don't do any of this. Do what I said the last time, which you marked as resolved without doing it or explaining. The only difficulty is that it requires the package name. But the only thing that's used for is to name the intl file. So you could derive the package name from the pubspec as before, but knowing that it's in the root directory and there's only one. Or you could derive it from the name of the intl.dart file, whose position is known.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanknight-wk I have updated the code

  1. To derive the package name from pubspec.yaml create the following method
  2. Also tested in ml_ui_client if we run this executable at root level where no pubspec.yaml this shows an error
    could not determine the package name but under ml_ui_client directory it works .
  3. Also tested in DPC where we have pubspec.yaml on root it not shows error but sorting not working because intl.dart files are under subpackage/package/lib/src eg: subpackage/datatables/lib/src

var basicDartPaths = parsedArgs.rest.isEmpty ? ['lib'] : parsedArgs.rest;
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
var dartPaths = [
for (var path in basicDartPaths) p.canonicalize(p.absolute(path))
];

// Work around parts being unresolved if you resolve them before their libraries.
// TODO - reference analyzer issue for this once it's created
final packageRoots = dartPaths.map(findPackageRootFor).toSet().toList();
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
packageRoots.sort((packageA, packageB) =>
p.split(packageB).length - p.split(packageA).length);

// TODO: Use packageConfig and utilities for reading that rather than manually parsing pubspec..
Map<String, String> packageNameLookup = {
for (String path in pubspecYamlPaths())
p.basename(findPackageRootFor(path)): fs
.file(path)
.readAsLinesSync()
.firstWhere((line) => line.startsWith('name'))
.split(':')
.last
.trim()
};

final processedPackages = Set<String>();
await pubGetForAllPackageRoots(dartPaths);
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved

// Is this necessary, or a duplicate of the earlier call? Like, do we have to run
// a null codemod again after the pub get?
exitCode = await runInteractiveCodemod(
[],
(_) async* {},
args: codemodArgs,
additionalHelpOutput: parser.usage,
);
if (exitCode != 0) return;
print('^ Ignore the "codemod found no files" warning above for now.');

for (String package in packageRoots) {
final packageRoot = p.basename(package);
final packageName = packageNameLookup[packageRoot] ?? 'fix_me_bad_name';
_log.info('Starting migration for $packageName');
Copy link
Contributor

Choose a reason for hiding this comment

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

The logger info doesn't actually print anything. I would just turn this into a print statement.
And change the text to something like Sorting and formatting INTL files for $packageName.

List<String> packageDartPaths;
try {
packageDartPaths =
dartFilesToMigrateForPackage(package, processedPackages).toList();
} on FileSystemException {
_log.info('${package} does not have a lib directory, moving on...');
return;
}

packageDartPaths = limitPaths(packageDartPaths, allowed: dartPaths);
sortPartsLast(packageDartPaths);

final IntlMessages messages = IntlMessages(packageName,
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
directory: fs.currentDirectory, packagePath: package);

exitCode =
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
await runMigrators(packageDartPaths, codemodArgs, messages, packageName);

processedPackages.add(package);

messages.write(force: parsedArgs[_noMigrate]);
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
messages.format();
}
}

void printUsage() {
stderr.writeln(
'Migrates literal strings that seem user-visible in the package by wrapping them in Intl.message calls.');
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
stderr.writeln();
stderr.writeln('Usage:');
stderr.writeln('intl_sorting [arguments]');
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
stderr.writeln();
stderr.writeln('Options:');
stderr.writeln(parser.usage);
}

/// Runs a set of codemod sequences separately to work around an issue where
/// updates from an earlier suggestor aren't reflected in the resolved AST
/// for later suggestors.
///
/// This means we have to set up analysis contexts multiple times, which takes longer,
/// but isn't a dealbreaker. E.g., for wdesk_sdk, running two sequences takes 2:52
/// as opposed to 2:00 for one sequence.
///
/// If any sequence fails, returns that exit code and short-circuits the other
/// sequences.
Future<int> runCodemodSequences(
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this method.

Iterable<String> paths,
Iterable<Iterable<Suggestor>> sequences,
List<String> codemodArgs,
) async {
for (final sequence in sequences) {
final exitCode = await runInteractiveCodemodSequence(
paths,
sequence,
defaultYes: true,
args: codemodArgs,
additionalHelpOutput: parser.usage,
);
if (exitCode != 0) return exitCode;
}

return 0;
}

/// Define your migrators here

// Example:
// final intlPropMigrator = IntlMigrator(messages.className, messages);

Future<int> runMigrators(List<String> packageDartPaths,
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is for running migration with only --no-migrate args

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, and I'm saying we shouldn't run migrators at all.

List<String> codemodArgs, IntlMessages messages, String packageName) async {
List<List<Migrator>> migrators = [
// if (parsedArgs[_noMigrate]) [intlPropMigrator],
// if (parsedArgs[_migrateConstants]) [constantStringMigrator],
// [displayNameMigrator],
// [importMigrator],
// if (parsedArgs[_migrateContextMenus]) [contextMenuMigrator],
];

List<List<Migrator>> thingsToRun = [
if (!parsedArgs[_noMigrate]) ...migrators,
// if (parsedArgs[_pruneUnused]) [usedMethodsChecker]
];

var result =
await runCodemodSequences(packageDartPaths, thingsToRun, codemodArgs);
return result;
}

void sortPartsLast(List<String> dartPaths) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this method.

akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
print('Sorting part files...');

final isPartCache = <String, bool>{};
bool isPart(String path) => isPartCache.putIfAbsent(path, () {
// parseString is much faster than using an AnalysisContextCollection
// to get unresolved AST, at least in repos with many context roots.
final unit = parseString(
content: LocalFileSystem().file(path).readAsStringSync())
.unit;
return unit.directives.whereType<PartOfDirective>().isNotEmpty;
});

if (dartPaths.isNotEmpty && dartPaths.every(isPart)) {
print(
'Only part files were specified. The containing library must be included for any part file, as it is needed for analysis context');
exit(1);
}
dartPaths.sort((a, b) {
final isAPart = isPart(a);
final isBPart = isPart(b);

if (isAPart == isBPart) return 0;
return isAPart ? 1 : -1;

});
print('Done.');

}

void sortDeepestFirst(Set<String> packageRoots) {
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
print('Sorting package roots...');

packageRoots
..toList().sort((packageA, packageB) => packageA.length - packageB.length)
..toSet();
}

Future<void> pubGetForAllPackageRoots(Iterable<String> files) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this method.

print(
'Running `pub get` if needed so that all Dart files can be resolved...');
final packageRoots = files.map(findPackageRootFor).toSet();
for (final packageRoot in packageRoots) {
await runPubGetIfNeeded(packageRoot);
}
}

Iterable<String> dartFilesToMigrate() => Glob('**.dart', recursive: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this method.

.listSync()
.whereType<File>()
.where((file) => !file.path.contains('.sg'))
.where((file) => !file.path.endsWith('_test.dart'))
.where(isNotHiddenFile)
.where(isNotDartHiddenFile)
.where(isNotWithinTopLevelBuildOutputDir)
.where(isNotWithinTopLevelToolDir)
.map((e) => e.path);

Iterable<String> dartFilesToMigrateForPackage(
String package, Set<String> processedPackages) =>
// Glob is peculiar about how it wants absolute Windows paths, so just query the
// file system directly. It wants "posix-style", but no leading slash. So
// C:/users/user/..., which is ugly to produce.
fs
.directory(p.join(package, 'lib'))
.listSync(recursive: true, followLinks: false)
.whereType<File>()
.where((file) => file.path.endsWith('.dart'))
.where((file) => !file.path.contains('.sg.g.dart'))
.where((file) => !file.path.contains('.sg.freezed.dart'))
.where((file) => !file.path.endsWith('_test.dart'))
.where((file) => !file.path.endsWith('_intl.dart'))
.where(isNotHiddenFile)
.where(isNotDartHiddenFile)
.where(isNotWithinTopLevelBuildOutputDir)
.where(isNotWithinTopLevelToolDir)
.where((file) => !processedPackages.contains(file.path))
.map((e) => e.path)
.toList();

Iterable<String> experienceConfigDartFiles() => [
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
for (var f in Glob('**.dart', recursive: true).listSync())
if (f is File && f.path.contains('_experience_config.dart')) f.path
];

// Limit the paths we handle to those that were included in [paths]
List<String> limitPaths(List<String> allPaths,
akshaybhardwaj-wk marked this conversation as resolved.
Show resolved Hide resolved
{required List<String> allowed}) =>
[
for (var path in allPaths)
if (allowed
.any((included) => included == path || p.isWithin(included, path)))
path
];

1 change: 1 addition & 0 deletions pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ executables:
rmui_preparation:
rmui_bundle_update:
intl_message_migration:
sort_intl:
dependency_validator:
ignore:
- meta
Expand Down
Loading