From 42132580e382c6639e54a7ad679cc82f0be348e3 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Mon, 16 Sep 2019 16:07:48 -0700 Subject: [PATCH] Adapt abstract component fixme comment logic to match the mixin stuff --- .../class_name_and_annotation_migrator.dart | 93 ++++++---- .../component2_constants.dart | 3 - lib/src/constants.dart | 8 + .../react16_suggestors/react16_utilities.dart | 44 ++++- ...ass_name_and_annotation_migrator_test.dart | 171 ++++++++++++------ 5 files changed, 221 insertions(+), 98 deletions(-) diff --git a/lib/src/component2_suggestors/class_name_and_annotation_migrator.dart b/lib/src/component2_suggestors/class_name_and_annotation_migrator.dart index f015aa9a0..446a9afef 100644 --- a/lib/src/component2_suggestors/class_name_and_annotation_migrator.dart +++ b/lib/src/component2_suggestors/class_name_and_annotation_migrator.dart @@ -18,7 +18,6 @@ import 'package:codemod/codemod.dart'; import 'package:over_react_codemod/src/react16_suggestors/react16_utilities.dart'; import '../constants.dart'; -import 'component2_constants.dart'; import 'component2_utilities.dart'; /// Suggestor that replaces `UiComponent` with `UiComponent2` in extends clauses @@ -87,6 +86,10 @@ class ClassNameAndAnnotationMigrator extends GeneralizingAstVisitor visitClassDeclaration(ClassDeclaration node) { super.visitClassDeclaration(node); + if (!fullyUpgradableToComponent2(node) && !allowPartialUpgrades) { + return; + } + if (!shouldUpgradeAbstractComponents && canBeExtendedFrom(node)) { return; } @@ -96,30 +99,6 @@ class ClassNameAndAnnotationMigrator extends GeneralizingAstVisitor return; } - if (!fullyUpgradableToComponent2(node)) { - if (!allowPartialUpgrades) return; - - if (hasOneOrMoreMixins(node)) { - // Ensure that this comment patch is idempotent. - final classHasAlreadyBeenVisited = extendsName.toString().endsWith('2'); - if (classHasAlreadyBeenVisited) return; - - final indentationLevel = node.beginToken.charOffset; - final commentLineBeginning = - indentationLevel == 0 ? '///' : (' ' * indentationLevel) + '///'; - - if (node.documentationComment != null) { - yieldPatch( - node.documentationComment.end, - node.documentationComment.end, - '\n$commentLineBeginning\n$commentLineBeginning FIXME: Before upgrading this component to `${extendsName}2`, verify that none of the mixin(s) contain implementations of any React lifecycle methods that are not supported in `${extendsName}2`.'); - } else { - yieldPatch(node.beginToken.offset, node.beginToken.offset, - '$commentLineBeginning FIXME: Before upgrading this component to `${extendsName}2`, verify that none of the mixin(s) contain implementations of any React lifecycle methods that are not supported in `${extendsName}2`.\n'); - } - } - } - String reactImportName = getImportNamespace(node, 'package:react/react.dart'); bool wasUpdated = false; @@ -162,10 +141,7 @@ class ClassNameAndAnnotationMigrator extends GeneralizingAstVisitor wasUpdated = true; }); - if (extendsName.name == 'UiComponent' || - extendsName.name == 'UiStatefulComponent' || - extendsName.name == 'FluxUiComponent' || - extendsName.name == 'FluxUiStatefulComponent') { + if (upgradableV1ComponentClassNames.contains(extendsName.name)) { // Update `UiComponent` or `UiStatefulComponent` extends clause. yieldPatch( extendsName.end, @@ -177,15 +153,56 @@ class ClassNameAndAnnotationMigrator extends GeneralizingAstVisitor } } - // Add comment for abstract components that are updated - if (wasUpdated && - canBeExtendedFrom(node) && - !hasComment(node, sourceFile, abstractClassMessage)) { - yieldPatch( - node.offset, - node.offset, - '$abstractClassMessage\n', - ); + if (wasUpdated) { + _addFixMeCommentPatches(node, extendsName); } } + + void _addFixMeCommentPatches(ClassDeclaration node, Identifier extendsName) { + final extendsNameStr = extendsName.toString().replaceAll('2', ''); + final classToUpgradeTo = + upgradableV1ComponentClassNames.contains(extendsNameStr) + ? '`${extendsNameStr}2`' + : 'a class that now extends from `UiComponent2`'; + + [ + _FixMeCommentPatch(hasOneOrMoreMixins, () => node, + 'FIXME: Before upgrading this component to $classToUpgradeTo, verify that none of the mixin(s) contain implementations of any React lifecycle methods that are not supported in $classToUpgradeTo.'), + _FixMeCommentPatch( + (node) => shouldUpgradeAbstractComponents && canBeExtendedFrom(node), + () => node, + 'FIXME: Abstract class has been updated to $classToUpgradeTo. This is a breaking change if this class is exported.'), + ].forEach((patch) { + final _node = patch.getNode(); + if (!patch.shouldYieldPatch(_node) || + hasMultilineDocComment(_node, sourceFile, patch.commentSrc)) { + return; + } + + final patchOffset = _node.metadata?.beginToken?.offset ?? + _node.firstTokenAfterCommentAndMetadata.offset; + yieldPatch(patchOffset, patchOffset, patch.commentSrc); + }); + } +} + +class _FixMeCommentPatch { + final bool Function(ClassDeclaration node) shouldYieldPatch; + final ClassDeclaration Function() getNode; + + _FixMeCommentPatch(this.shouldYieldPatch, this.getNode, String commentSrc) { + final node = getNode(); + final indentationLevel = node.beginToken.charOffset; + final fixmeCommentLineStart = + indentationLevel == 0 ? '///' : (' ' * indentationLevel) + '///'; + final fixmeCommentStart = node.documentationComment != null + ? '$fixmeCommentLineStart\n$fixmeCommentLineStart' + : fixmeCommentLineStart; + final fixmeCommentEnd = '\n'; + + _commentSrc = '$fixmeCommentStart $commentSrc$fixmeCommentEnd'; + } + + String _commentSrc; + String get commentSrc => _commentSrc; } diff --git a/lib/src/component2_suggestors/component2_constants.dart b/lib/src/component2_suggestors/component2_constants.dart index f65c3cbfa..6a5c2334f 100644 --- a/lib/src/component2_suggestors/component2_constants.dart +++ b/lib/src/component2_suggestors/component2_constants.dart @@ -35,6 +35,3 @@ String getDeperecationMessage(String methodName) { /// $revertInstructions'''; } - -const abstractClassMessage = - '/// FIXME: Abstract class has been updated to Component2. This is a breaking change if this class is exported.'; diff --git a/lib/src/constants.dart b/lib/src/constants.dart index 45d86155e..fe29833df 100644 --- a/lib/src/constants.dart +++ b/lib/src/constants.dart @@ -72,6 +72,14 @@ const List overReactMixinAnnotationNames = [ 'StateMixin', ]; +/// A list of the names of the core component classes that can be upgraded to a "v2" version. +const List upgradableV1ComponentClassNames = [ + 'UiComponent', + 'UiStatefulComponent', + 'FluxUiComponent', + 'FluxUiStatefulComponent', +]; + /// Dart type for the static meta field on props classes. const String propsMetaType = 'PropsMeta'; diff --git a/lib/src/react16_suggestors/react16_utilities.dart b/lib/src/react16_suggestors/react16_utilities.dart index 265237e46..03f282ea1 100644 --- a/lib/src/react16_suggestors/react16_utilities.dart +++ b/lib/src/react16_suggestors/react16_utilities.dart @@ -51,13 +51,55 @@ bool hasComment(AstNode node, SourceFile sourceFile, String comment) { return commentText?.contains(comment) ?? false; } +/// Whether the [node] has a documentation comment that has +/// any lines that match lines found within the provided [comment]. +bool hasMultilineDocComment( + AnnotatedNode node, SourceFile sourceFile, String comment) { + final nodeComments = nodeCommentSpan(node, sourceFile) + .text + .replaceAll('///', '') + .split('\n') + .map((line) => line.replaceAll('\n', '').trim()) + .toList() + ..removeWhere((line) => line.isEmpty); + final commentLines = comment + .replaceAll('///', '') + .trimLeft() + .split('\n') + .map((line) => line.replaceAll('\n', '').trim()) + .toList() + ..removeWhere((line) => line.isEmpty); + + bool match = false; + + for (var i = 0; i < commentLines.length; i++) { + final potentialMatch = commentLines[i]; + if (nodeComments.any((line) => line == potentialMatch)) { + match = true; + print( + '\n\nMATCH! ${nodeComments.where((line) => line == potentialMatch)}'); + break; + } + } + + return match; +} + +/// Returns the `SourceSpan` value of any comments on the provided [node] within the [sourceFile]. +SourceSpan nodeCommentSpan(AnnotatedNode node, SourceFile sourceFile) { + return sourceFile.span( + node.beginToken.offset, + node.metadata?.beginToken?.offset ?? + node.firstTokenAfterCommentAndMetadata.offset); +} + /// Returns an iterable of all the comments from [beginToken] to the end of the /// file. /// /// Comments are part of the normal stream, and need to be accessed via /// [Token.precedingComments], so it's difficult to iterate over them without /// this method. -Iterable allComments(Token beginToken) sync* { +Iterable allComments(Token beginToken) sync* { var currentToken = beginToken; while (!currentToken.isEof) { var currentComment = currentToken.precedingComments; diff --git a/test/component2_suggestors/class_name_and_annotation_migrator_test.dart b/test/component2_suggestors/class_name_and_annotation_migrator_test.dart index 47dd6abc3..657111168 100644 --- a/test/component2_suggestors/class_name_and_annotation_migrator_test.dart +++ b/test/component2_suggestors/class_name_and_annotation_migrator_test.dart @@ -13,7 +13,6 @@ // limitations under the License. import 'package:over_react_codemod/src/component2_suggestors/class_name_and_annotation_migrator.dart'; -import 'package:over_react_codemod/src/component2_suggestors/component2_constants.dart'; import 'package:test/test.dart'; import '../util.dart'; @@ -232,53 +231,113 @@ void classNameAndAnnotationTests({ }); } else { group('adds a FIXME comment when one or more mixins are present', () { - test('and a documentation comment is already present', () { - testSuggestor( - expectedPatchCount: 3, - input: ''' - /// This is my current documentation comment. - /// - /// As you can see it has more than one line, which is pretty great! - @Component() - class FooComponent extends UiComponent with FooMixin { - @override - render() {} - } - ''', - expectedOutput: ''' - /// This is my current documentation comment. - /// - /// As you can see it has more than one line, which is pretty great! - /// - /// FIXME: Before upgrading this component to `UiComponent2`, verify that none of the mixin(s) contain implementations of any React lifecycle methods that are not supported in `UiComponent2`. - @Component2() - class FooComponent extends UiComponent2 with FooMixin { - @override - render() {} - } - ''', - ); + group('and a documentation comment is already present', () { + test('', () { + testSuggestor( + expectedPatchCount: 3, + input: ''' + /// This is my current documentation comment. + /// + /// As you can see it has more than one line, which is pretty great! + @Component() + class FooComponent extends UiComponent with FooMixin { + @override + render() {} + } + ''', + expectedOutput: ''' + /// This is my current documentation comment. + /// + /// As you can see it has more than one line, which is pretty great! + /// + /// FIXME: Before upgrading this component to `UiComponent2`, verify that none of the mixin(s) contain implementations of any React lifecycle methods that are not supported in `UiComponent2`. + @Component2() + class FooComponent extends UiComponent2 with FooMixin { + @override + render() {} + } + ''', + ); + }); + + if (shouldUpgradeAbstractComponents) { + test('on an abstract component', () { + testSuggestor( + expectedPatchCount: 4, + input: ''' + /// This is my current documentation comment. + /// + /// As you can see it has more than one line, which is pretty great! + @AbstractComponent() + abstract class FooComponent extends UiComponent with FooMixin { + @override + render() {} + } + ''', + expectedOutput: ''' + /// This is my current documentation comment. + /// + /// As you can see it has more than one line, which is pretty great! + /// + /// FIXME: Before upgrading this component to `UiComponent2`, verify that none of the mixin(s) contain implementations of any React lifecycle methods that are not supported in `UiComponent2`. + /// + /// FIXME: Abstract class has been updated to `UiComponent2`. This is a breaking change if this class is exported. + @AbstractComponent2() + abstract class FooComponent extends UiComponent2 with FooMixin { + @override + render() {} + } + ''', + ); + }); + } }); - test('and no documentation comment is present', () { - testSuggestor( - expectedPatchCount: 3, - input: ''' - @Component() - class FooComponent extends UiComponent with FooMixin { - @override - render() {} - } - ''', - expectedOutput: ''' - /// FIXME: Before upgrading this component to `UiComponent2`, verify that none of the mixin(s) contain implementations of any React lifecycle methods that are not supported in `UiComponent2`. - @Component2() - class FooComponent extends UiComponent2 with FooMixin { - @override - render() {} - } - ''', - ); + group('and no documentation comment is present', () { + test('', () { + testSuggestor( + expectedPatchCount: 3, + input: ''' + @Component() + class FooComponent extends UiComponent with FooMixin { + @override + render() {} + } + ''', + expectedOutput: ''' + /// FIXME: Before upgrading this component to `UiComponent2`, verify that none of the mixin(s) contain implementations of any React lifecycle methods that are not supported in `UiComponent2`. + @Component2() + class FooComponent extends UiComponent2 with FooMixin { + @override + render() {} + } + ''', + ); + }); + + if (shouldUpgradeAbstractComponents) { + test('on an abstract component', () { + testSuggestor( + expectedPatchCount: 4, + input: ''' + @AbstractComponent() + abstract class FooComponent extends UiComponent with FooMixin { + @override + render() {} + } + ''', + expectedOutput: ''' + /// FIXME: Before upgrading this component to `UiComponent2`, verify that none of the mixin(s) contain implementations of any React lifecycle methods that are not supported in `UiComponent2`. + /// FIXME: Abstract class has been updated to `UiComponent2`. This is a breaking change if this class is exported. + @AbstractComponent2() + abstract class FooComponent extends UiComponent2 with FooMixin { + @override + render() {} + } + ''', + ); + }); + } }); test('and the component extends UiStatefulComponent', () { @@ -446,7 +505,7 @@ void classNameAndAnnotationTests({ } ''', expectedOutput: ''' - ${shouldUpgradeAbstractComponents ? abstractClassMessage : ''} + ${shouldUpgradeAbstractComponents ? '/// FIXME: Abstract class has been updated to `UiStatefulComponent2`. This is a breaking change if this class is exported.' : ''} @AbstractComponent${shouldUpgradeAbstractComponents ? '2' : ''}(isWrapper: true) abstract class FooComponent extends UiStatefulComponent${shouldUpgradeAbstractComponents ? '2' : ''} { @override @@ -473,7 +532,7 @@ void classNameAndAnnotationTests({ } ''', expectedOutput: ''' - ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? abstractClassMessage : ''} + ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '/// FIXME: Abstract class has been updated to `FluxUiStatefulComponent2`. This is a breaking change if this class is exported.' : ''} @AbstractComponent${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '2' : ''}(isWrapper: true) abstract class FooComponent extends FluxUiStatefulComponent${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '2' : ''} { @override @@ -497,7 +556,7 @@ void classNameAndAnnotationTests({ abstract class FooComponent extends SomeOtherClass {} ''', expectedOutput: ''' - ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? abstractClassMessage : ''} + ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '/// FIXME: Abstract class has been updated to a class that now extends from `UiComponent2`. This is a breaking change if this class is exported.' : ''} @AbstractComponent${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '2' : ''}(isWrapper: true) abstract class FooComponent extends SomeOtherClass {} ''', @@ -519,7 +578,7 @@ void classNameAndAnnotationTests({ } ''', expectedOutput: ''' - ${shouldUpgradeAbstractComponents ? abstractClassMessage : ''} + ${shouldUpgradeAbstractComponents ? '/// FIXME: Abstract class has been updated to `UiStatefulComponent2`. This is a breaking change if this class is exported.' : ''} @Component${shouldUpgradeAbstractComponents ? '2' : ''} class FooComponent extends UiStatefulComponent${shouldUpgradeAbstractComponents ? '2' : ''} { @override @@ -546,7 +605,7 @@ void classNameAndAnnotationTests({ } ''', expectedOutput: ''' - ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? abstractClassMessage : ''} + ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '/// FIXME: Abstract class has been updated to `FluxUiComponent2`. This is a breaking change if this class is exported.' : ''} @Component${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '2' : ''} class FooComponent extends FluxUiComponent${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '2' : ''} { @override @@ -570,7 +629,7 @@ void classNameAndAnnotationTests({ class FooComponent extends SomeOtherClass {} ''', expectedOutput: ''' - ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? abstractClassMessage : ''} + ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '/// FIXME: Abstract class has been updated to a class that now extends from `UiComponent2`. This is a breaking change if this class is exported.' : ''} @Component${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '2' : ''} class FooComponent extends SomeOtherClass {} ''', @@ -598,7 +657,7 @@ void classNameAndAnnotationTests({ @AbstractProps() class AbstractFooProps extends UiProps {} - ${shouldUpgradeAbstractComponents ? abstractClassMessage : ''} + ${shouldUpgradeAbstractComponents ? '/// FIXME: Abstract class has been updated to `UiStatefulComponent2`. This is a breaking change if this class is exported.' : ''} @AbstractComponent${shouldUpgradeAbstractComponents ? '2' : ''}() class FooComponent extends UiStatefulComponent${shouldUpgradeAbstractComponents ? '2' : ''} { @override @@ -631,7 +690,7 @@ void classNameAndAnnotationTests({ @AbstractProps() class AbstractFooProps extends UiProps {} - ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? abstractClassMessage : ''} + ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '/// FIXME: Abstract class has been updated to `FluxUiStatefulComponent2`. This is a breaking change if this class is exported.' : ''} @AbstractComponent${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '2' : ''}() class FooComponent extends FluxUiStatefulComponent${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '2' : ''} { @override @@ -661,7 +720,7 @@ void classNameAndAnnotationTests({ @AbstractProps() class AbstractFooProps extends UiProps {} - ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? abstractClassMessage : ''} + ${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '/// FIXME: Abstract class has been updated to a class that now extends from `UiComponent2`. This is a breaking change if this class is exported.' : ''} @AbstractComponent${allowPartialUpgrades && shouldUpgradeAbstractComponents ? '2' : ''}() class FooComponent extends SomeOtherClass {} ''', @@ -673,7 +732,7 @@ void classNameAndAnnotationTests({ testSuggestor( expectedPatchCount: 0, input: ''' - $abstractClassMessage + /// FIXME: Abstract class has been updated to a class that now extends from `UiStatefulComponent2`. This is a breaking change if this class is exported. @AbstractComponent2(isWrapper: true) abstract class FooComponent extends UiStatefulComponent2 { @override