AF-3838 Replace python implementation with dart using dart_codemod #12

evanweible-wf · 2019-01-07T07:08:15Z

Description

The python version of this tool fell short and was unable to handle all the necessary edge cases. Now that we have dart_codemod, this tool should be ported to dart.

This PR adds the first of 3 codemods:

dart1_to_dart1_and_dart2

As well as a readme and tests.

Testing

CI passes
Codemod can be used to roll out changes to an over_react consumer

Code Review

@corwinsheahan-wf @sebastianmalysa-wf @georgelesica-wf @smaifullerton-wk

rmconsole-wf · 2019-01-07T07:08:31Z

Merge Requirements Met ✅

Request Rosie to automerge this pull request by including @Workiva/release-management-p in a comment.

General Information

Ticket(s):

AF-3838 | remove

Code Review(s): #12
Release Image Tags:

Reviewers: corwinsheahan-wf, smaifullerton-wk, sebastianmalysa-wf

Additional Information

Watchlist Notifications: None

	When this pull is merged I will add it to the following release:
	Version: over_react_codemod 0.1.0
	Release Ticket(s): None

Note: This is a shortened report. Click here to view Rosie's full evaluation.
Last updated on Wednesday, January 16 02:59 PM CST

aviary2-wf · 2019-01-07T07:08:33Z

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.

corwinsheahan-wf

Almost done, I'll finish up tomorrow. This looks awesome!

corwinsheahan-wf · 2019-01-07T22:51:22Z

lib/src/d1_to_d1_and_d2/suggestors/props_and_state_mixin_meta_adder.dart

+      return;
+    }
+
+    final targetMetaInitializer = '\$metaFor${node.name.name}';


should we pass node.name.name to stripPrivateGeneratedPrefix first?

Since this is for the Dart1 -> Dart1/Dart2 migration, and since this is only visiting props and state mixins, there shouldn't ever be a _$ prefix.. but it wouldn't hurt and it would be a good reminder when writing the dart2-only migration

corwinsheahan-wf · 2019-01-07T22:52:45Z

lib/src/d1_to_d1_and_d2/suggestors/props_and_state_mixin_meta_adder.dart

+      );
+    } else {
+      // Meta field already exists.
+      if (existingMetaField.initializer.toSource() != targetMetaInitializer) {


should we instead error here? Thinking of the case where the user has defined the meta field already as something else. Then the user gets a heads up that something is not right.

corwinsheahan-wf · 2019-01-07T22:57:24Z

lib/src/d1_to_d1_and_d2/suggestors/props_and_state_mixin_usage_updater.dart

+    implements Suggestor {
+  @override
+  visitWithClause(WithClause node) {
+    final allMixins = node.mixinTypes.map((n) => n.name.name);


In reference to the TODO above, can we look for the metadata off of any mixin node and determine whether it contains an over_react mixin annotation?

Talked about this offline, but this isn't possible with an AstVisitor. We'd need to use an ElementVisitor, which is something I'd like to pursue but is more complicated because we have to setup an AnalysisDriver in order to resolve all of the elements.

corwinsheahan-wf · 2019-01-07T22:58:55Z

lib/src/d1_to_d1_and_d2/suggestors/ui_factory_initializer.dart

+
+  static getFactoryInitializerValue(String factoryName) {
+    if (factoryName.startsWith(privatePrefix)) {
+      final factoryNameWithoutUnderscore =


#nit factoryNameWithoutPrivatePrefix

corwinsheahan-wf · 2019-01-07T23:00:39Z

lib/src/d1_to_d1_and_d2/suggestors/ui_factory_initializer.dart

+      return;
+    }
+
+    if (factoryNode.initializer != null) {


This is another situation that I'm not sure we shouldn't error on.

We talked about this offline, and decided that we will error if there is an unexpected value in this initializer (same thing for if the meta field is already defined or initialized with something unexpected. If it's an incorrect value, but one that we would expect from a previous iteration of the codemod, then it will just update it.

corwinsheahan-wf · 2019-01-08T00:23:29Z

test/d1_to_d1_and_d2/needs_over_react_library_collector_test.dart

+      // Part library that uses over_react.
+      final partLib = './lib/part.dart';
+      final parentLib = './lib/parent.dart';
+      final partSourceFile = SourceFile.fromString('''part of 'parent.dart';


We should also run the collector on the parent source file in this test to ensure that there is not a duplicity in the libraries detected.

corwinsheahan-wf · 2019-01-08T00:41:30Z

test/d1_to_d1_and_d2/suggestor_tests/props_and_state_classes_renamer.suggestor_test

+>>> @Props() private (patches 1)
+@Props() class _Foo {}
+<<<
+@Props() class _$_Foo {}


~~Private classes should be _$Foo, not _$_Foo.~~ this statement is now incorrect. Private classes should be _$_Foo

corwinsheahan-wf · 2019-01-08T00:49:15Z

test/d1_to_d1_and_d2/suggestor_tests/props_and_state_companion_class_adder.suggestor_test

+class _Foo extends _$_Foo
+    with
+        // ignore: mixin_of_non_class, undefined_class
+        _$_FooAccessorsMixin {


~~This should also be _$FooAccessorsMixin~~ it should be _$_FooAccessorsMixin

corwinsheahan-wf · 2019-01-08T00:50:01Z

test/d1_to_d1_and_d2/suggestor_tests/props_and_state_companion_class_adder.suggestor_test

+        // ignore: mixin_of_non_class, undefined_class
+        _$_FooAccessorsMixin {
+  // ignore: const_initialized_with_non_constant_value, undefined_class, undefined_identifier
+  static const PropsMeta meta = $metaFor_Foo;


~~This should be $metaForFoo~~. Should be _$metaFor_Foo

@evanweible-wf this comment reflects a needed change^

corwinsheahan-wf · 2019-01-08T00:58:50Z

test/d1_to_d1_and_d2/suggestor_tests/props_and_state_companion_class_adder.suggestor_test

+
+>>> @State() retains doc comments (patches 1)
+/// A doc comment.
+/// And a second line!


Can we get 1 more test which retains both the doc comment and the metadata?

corwinsheahan-wf

ok I'm done now :)

corwinsheahan-wf · 2019-01-08T17:31:55Z

test/util_test.dart

+        });
+      }
+
+      test('returns a false negative when annotation found in a string', () {


why not just:

Suggested change

test('returns a false negative when annotation found in a string', () {

test('returns true when annotation found in a string', () {

I was trying to make it clear that this is a known limitation in the implementation 🤷‍♂️

- Added a readme - Updated expected initializer values for static meta fields and UI factories - Added support for ignoring patch suggestions via single-line comments - Added support for a --comment-prefix CLI option

evanweible-wf · 2019-01-15T15:48:56Z

@corwinsheahan-wf addressed most of your comments. I'm going to leave out some of the error-throwing that we talked about for now because I think it will be mainly a corner case. We can add it as a follow up if need be.

sebastianmalysa-wf · 2019-01-16T16:34:01Z

README.md

+- `pub global run over_react_codemod:dart1_and_dart2_to_dart2`
+
+    Use this codemod to migrate over_react code that has previously been
+    migrated via the `dart1_to_dart1_and_dart1` codemod to the form that is only


#nit should this be ...'dart1_to_dart1_and_dart2'?

Yup, good catch

corwinsheahan-wf · 2019-01-16T17:01:27Z

lib/src/d1_to_d1_and_d2/util.dart

+class $strippedClassName$typeParamsOnClass extends ${privateGeneratedPrefix}$strippedClassName$typeParamsOnSuper
+    with
+        $mixinIgnoreComment
+        ${privateGeneratedPrefix}${strippedClassName}AccessorsMixin {


Suggested change

${privateGeneratedPrefix}${strippedClassName}AccessorsMixin {

${privateGeneratedPrefix}${strippedClassName}AccessorsMixin$typeParamsOnSuper {

smaifullerton-wk · 2019-01-16T18:15:59Z

QA +1

CI passes
Codemod can be used to roll out changes to an over_react consumer

I installed locally from this branch and tested with review_bar, looks good:

corwinsheahan-wf · 2019-01-16T18:33:32Z

@Workiva/release-management-p

rmconsole-wf · 2019-01-16T18:33:51Z

@corwinsheahan-wf I will not merge this because:

untracked dependencies

evanweible-wf · 2019-01-16T20:59:13Z

QA +1

CI passes
Previous +10 still stands as the last two commits did not change any source code AF-3838 Replace python implementation with dart using dart_codemod #12 (comment)

@Workiva/release-management-p

rmconsole-wf · 2019-01-16T20:59:18Z

+1 from RM

Delete python version of codemod.

00f8644

rmconsole-wf added the Merge Requirements Unmet label Jan 7, 2019

Add Dart version of codemod.

450c154

evanweible-wf force-pushed the dart branch from bc5cd61 to 450c154 Compare January 7, 2019 07:12

Handle generic type params/args better. More tests.

a318dfd

corwinsheahan-wf requested changes Jan 8, 2019

View reviewed changes

corwinsheahan-wf reviewed Jan 8, 2019

View reviewed changes

evanweible-wf added 4 commits January 14, 2019 15:59

Several updates:

3122b34

- Added a readme - Updated expected initializer values for static meta fields and UI factories - Added support for ignoring patch suggestions via single-line comments - Added support for a --comment-prefix CLI option

Add PubspecOverReactUpgrader suggestor.

37296a1

Add args dep

c440f24

Add a test for retaining generics and doc comments

3bdec48

sebastianmalysa-wf reviewed Jan 16, 2019

View reviewed changes

sebastianmalysa-wf approved these changes Jan 16, 2019

View reviewed changes

corwinsheahan-wf requested changes Jan 16, 2019

View reviewed changes

evanweible-wf added 2 commits January 16, 2019 10:03

Also apply generic args to AccessorsMixin

a182e5f

Fix typo in readme.

9c70959

corwinsheahan-wf approved these changes Jan 16, 2019

View reviewed changes

evanweible-wf changed the title ~~WIP Replace python implementation with dart using dart_codemod~~ AF-3838 Replace python implementation with dart using dart_codemod Jan 16, 2019

sebastianmalysa-wf approved these changes Jan 16, 2019

View reviewed changes

rmconsole-wf added the RM Ready label Jan 16, 2019

evanweible-wf added 2 commits January 16, 2019 12:39

Run ddev format in --check mode

1d1585f

Use published version of codemod

2a5701c

corwinsheahan-wf approved these changes Jan 16, 2019

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Jan 16, 2019

rmconsole7-wk merged commit 28ed4ae into master Jan 16, 2019

rmconsole7-wk deleted the dart branch January 16, 2019 20:59

rmconsole-readonly-wk mentioned this pull request Jan 23, 2019

AF-3863 Release over_react_codemod 1.0.0 #15

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AF-3838 Replace python implementation with dart using dart_codemod #12

AF-3838 Replace python implementation with dart using dart_codemod #12

evanweible-wf commented Jan 7, 2019 •

edited

Loading

rmconsole-wf commented Jan 7, 2019 •

edited by rmconsole7-wk

Loading

aviary2-wf commented Jan 7, 2019

corwinsheahan-wf left a comment

corwinsheahan-wf Jan 7, 2019

evanweible-wf Jan 10, 2019

corwinsheahan-wf Jan 7, 2019

corwinsheahan-wf Jan 7, 2019

evanweible-wf Jan 10, 2019

corwinsheahan-wf Jan 7, 2019

corwinsheahan-wf Jan 7, 2019

evanweible-wf Jan 10, 2019

corwinsheahan-wf Jan 8, 2019

corwinsheahan-wf Jan 8, 2019 •

edited

Loading

corwinsheahan-wf Jan 8, 2019 •

edited

Loading

corwinsheahan-wf Jan 8, 2019 •

edited

Loading

corwinsheahan-wf Jan 10, 2019

corwinsheahan-wf Jan 8, 2019

corwinsheahan-wf left a comment

corwinsheahan-wf Jan 8, 2019

evanweible-wf Jan 9, 2019

evanweible-wf commented Jan 15, 2019

sebastianmalysa-wf Jan 16, 2019

evanweible-wf Jan 16, 2019

corwinsheahan-wf Jan 16, 2019

smaifullerton-wk commented Jan 16, 2019

corwinsheahan-wf commented Jan 16, 2019

rmconsole-wf commented Jan 16, 2019

evanweible-wf commented Jan 16, 2019

rmconsole-wf commented Jan 16, 2019

	test('returns a false negative when annotation found in a string', () {
	test('returns true when annotation found in a string', () {

	${privateGeneratedPrefix}${strippedClassName}AccessorsMixin {
	${privateGeneratedPrefix}${strippedClassName}AccessorsMixin$typeParamsOnSuper {

AF-3838 Replace python implementation with dart using dart_codemod #12

AF-3838 Replace python implementation with dart using dart_codemod #12

Conversation

evanweible-wf commented Jan 7, 2019 • edited Loading

Description

Testing

Code Review

rmconsole-wf commented Jan 7, 2019 • edited by rmconsole7-wk Loading

Merge Requirements Met ✅

General Information

Additional Information

aviary2-wf commented Jan 7, 2019

Security Insights

Action Items

corwinsheahan-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corwinsheahan-wf Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

corwinsheahan-wf Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

corwinsheahan-wf Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corwinsheahan-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanweible-wf commented Jan 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaifullerton-wk commented Jan 16, 2019

corwinsheahan-wf commented Jan 16, 2019

rmconsole-wf commented Jan 16, 2019

evanweible-wf commented Jan 16, 2019

rmconsole-wf commented Jan 16, 2019

evanweible-wf commented Jan 7, 2019 •

edited

Loading

rmconsole-wf commented Jan 7, 2019 •

edited by rmconsole7-wk

Loading

corwinsheahan-wf Jan 8, 2019 •

edited

Loading

corwinsheahan-wf Jan 8, 2019 •

edited

Loading

corwinsheahan-wf Jan 8, 2019 •

edited

Loading