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

CPLAT-9308 Codemod Utility to tell if Something is Public API #74

Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
53d0862
Create SemverHelper class
sydneyjodon-wk Jan 30, 2020
4900548
Create utility function
sydneyjodon-wk Jan 30, 2020
070c5f9
Format
sydneyjodon-wk Jan 30, 2020
723a406
Add SemverHelper constructor
sydneyjodon-wk Feb 4, 2020
cadf6ae
Add doc comment
sydneyjodon-wk Feb 4, 2020
d806385
Add tests
sydneyjodon-wk Feb 4, 2020
c5e7e74
Merge branch 'CPLAT-9205-new-boilerplate-codemod' into CPLAT-9308-get…
sydneyjodon-wk Feb 4, 2020
7027deb
Merge base branch in
sydneyjodon-wk Feb 4, 2020
900a557
Update migrator tests
sydneyjodon-wk Feb 4, 2020
7c1f44e
Remove unused imports
sydneyjodon-wk Feb 4, 2020
457deb6
Merge CPLAT-9205-new-boilerplate-codemod into CPLAT-9308-getPublicExp…
aaronlademann-wf Feb 5, 2020
f9d81d9
Merge remote-tracking branch 'origin/CPLAT-9205-new-boilerplate-codem…
aaronlademann-wf Feb 5, 2020
8abec86
Merge branch 'CPLAT-9205-new-boilerplate-codemod' into CPLAT-9308-get…
sydneyjodon-wk Feb 6, 2020
c61a5c4
Address reviewer feedback
sydneyjodon-wk Feb 6, 2020
d53c197
Remove duplicate import
sydneyjodon-wk Feb 6, 2020
82e39f7
Add doc comment
sydneyjodon-wk Feb 6, 2020
c5430ab
Merge branch 'CPLAT-9205-new-boilerplate-codemod' into CPLAT-9308-get…
sydneyjodon-wk Feb 6, 2020
7d844b0
Fix errors from merge
sydneyjodon-wk Feb 6, 2020
5783dcb
Address reviewer feedback
sydneyjodon-wk Feb 11, 2020
23dc002
Merge branch 'CPLAT-9205-new-boilerplate-codemod' into CPLAT-9308-get…
sydneyjodon-wk Feb 11, 2020
8775714
Address reviewer feedback
sydneyjodon-wk Feb 11, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,19 @@ import 'boilerplate_utilities.dart';
class AdvancedPropsAndStateClassMigrator extends GeneralizingAstVisitor
with AstVisitingSuggestorMixin
implements Suggestor {
final SemverHelper helper;

AdvancedPropsAndStateClassMigrator(this.helper);

@override
visitClassDeclaration(ClassDeclaration node) {
super.visitClassDeclaration(node);

if (!shouldMigrateAdvancedPropsAndStateClass(node)) return;
if (!shouldMigrateAdvancedPropsAndStateClass(node, helper)) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This suggestion builds on code that isn't here yet (it would come from Aaron's branches), but how would you feel about integrating the helper into an isPublic function? I think the implementation could be something like:

boilerplate_utilities.dart

// Create a helper that the utilities can see
SemverHelper semverHelper;

bool isPublic(ClassDeclaration node) {
  assert(semverHelper != null);
  return semverHelper.getPublicExportLocations(node) != null;
}

bool shouldMigratePropsAndStateClass(
    ClassDeclaration node) {
  return isAssociatedWithComponent2(node) &&
      isAPropsOrStateClass(node) && !isPublic(node);

boilerplate_upgrade.dart

semverHelper = SemverHelper(jsonDecode(
      await File('lib/src/boilerplate_suggestors/report.json').readAsString()));

The main thing I'm trying to prevent is passing around the same helper instance through all the suggestors and utilities, but if that seems like a good goal then I'm open to whatever implementation works best!

Copy link
Contributor

Choose a reason for hiding this comment

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

The main thing I'm trying to prevent is passing around the same helper instance through all the suggestors and utilities, but if that seems like a good goal then I'm open to whatever implementation works best!

Passing around the instance is actually what I'd prefer, as opposed to a global variable.

Advantages:

  • Eliminates the risk of different areas of the code unintentionally interfering with each other's data (including in tests)
    -Make it easier to reason about where this is used, and force us to be intentional about where we pass it

Copy link
Contributor

Choose a reason for hiding this comment

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

I could definitely see that, and normally I wouldn't advocate for a global variable, but my main thought is that the function which answers the question of whether a node is public or not is at the utility level, and it doesn't seem like the suggestor should need to be concerned with how it answers that question (outside of passing in the node).

Also, because this helper should receive the report for the relevant library it's running on, it seems like it makes sense for it to operate independently since any instantiation for an executable should be passing it the same report. For tests that does break down, but then the testing environment could have its own instance when needed. If you feel strongly, Greg, I think that generally global variables are bad, this is just a weird case to me 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel pretty strongly here that moving away from a top-level variable would improve the modularity of this code, as well as the predictability of how data is passed around and used.

it seems like it makes sense for it to operate independently since any instantiation for an executable should be passing it the same report

That's true, but may not always be true. For example, what if we wanted this utility to behave slightly differently for different suggestors? Or if we wanted to run multiple sets of suggestors on different directories concurrently? Having global variables would make it much more difficult to implement these changes. To be fair, this is the weakest argument of the ones I've presented, but it is something to keep in mind.


I'm happy to make a PR with those changes, especially since some work was already done to move it more toward top-level variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving away from a global variable sounds good to me. Thanks for explaining your logic, Greg! And sorry for making ya change it in the first place Sydney 😅

}
}

bool shouldMigrateAdvancedPropsAndStateClass(ClassDeclaration node) =>
shouldMigratePropsAndStateClass(node) && isAdvancedPropsOrStateClass(node);
bool shouldMigrateAdvancedPropsAndStateClass(
ClassDeclaration node, SemverHelper helper) =>
shouldMigratePropsAndStateClass(node, helper) &&
isAdvancedPropsOrStateClass(node);
6 changes: 6 additions & 0 deletions lib/src/boilerplate_suggestors/annotations_remover.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,16 @@ import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:codemod/codemod.dart';

import 'boilerplate_utilities.dart';

/// Suggestor that looks for @Props, @State, and @Component2 and removes them.
class AnnotationsRemover extends GeneralizingAstVisitor
with AstVisitingSuggestorMixin
implements Suggestor {
final SemverHelper helper;

AnnotationsRemover(this.helper);

@override
visitAnnotatedNode(AnnotatedNode node) {
super.visitAnnotatedNode(node);
Expand Down
35 changes: 29 additions & 6 deletions lib/src/boilerplate_suggestors/boilerplate_utilities.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,12 @@ import 'package:over_react_codemod/src/util.dart';
typedef void YieldPatch(
int startingOffset, int endingOffset, String replacement);

// Stub while <https://jira.atl.workiva.net/browse/CPLAT-9308> is in progress
bool _isPublic(ClassDeclaration node) => false;

/// Whether a props or state class class [node] should be migrated as part of the boilerplate codemod.
bool shouldMigratePropsAndStateClass(ClassDeclaration node) {
bool shouldMigratePropsAndStateClass(
ClassDeclaration node, SemverHelper helper) {
return isAssociatedWithComponent2(node) &&
isAPropsOrStateClass(node) &&
// Stub while <https://jira.atl.workiva.net/browse/CPLAT-9308> is in progress
!_isPublic(node);
helper.getPublicExportLocations(node) == null;
}

/// A simple RegExp against the parent of the class to verify it is `UiProps`
Expand Down Expand Up @@ -119,3 +116,29 @@ void migrateClassToMixin(ClassDeclaration node, YieldPatch yieldPatch,
propsAndStateClassNamesConvertedToNewBoilerplate[originalPublicClassName] =
newMixinName;
}

class SemverHelper {
Map _exportList;

SemverHelper(Map jsonReport) {
_exportList = jsonReport['exports'];
}

/// Returns semver report information for [node] if it is publicly exported.
///
/// If [node] is not publicly exported, returns `null`.
Map<String, dynamic> getPublicExportLocations(ClassDeclaration node) {
final className = node?.name?.name;
String classKey;

if (className == null || _exportList == null) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we may want this to be a louder failure. Have we seen the class name be null before? I'd be curious to see what that code looks like haha

If these can be null though, I'd expect it to throw an Exception since we need the list and class name to really verify if the class is exported, and returning null could be a false negative


_exportList.forEach((key, value) {
if (value['type'] == 'class' && value['grammar']['name'] == className) {
classKey = key;
}
});
sydneyjodon-wk marked this conversation as resolved.
Show resolved Hide resolved

return _exportList[classKey];
}
}
5 changes: 5 additions & 0 deletions lib/src/boilerplate_suggestors/props_mixins_migrator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,16 @@
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:codemod/codemod.dart';
import 'package:over_react_codemod/src/boilerplate_suggestors/boilerplate_utilities.dart';

/// Suggestor that updates stand alone props mixins to be actual mixins.
class PropsMixinMigrator extends GeneralizingAstVisitor
with AstVisitingSuggestorMixin
implements Suggestor {
final SemverHelper helper;

PropsMixinMigrator(this.helper);

@override
visitClassDeclaration(ClassDeclaration node) {
super.visitClassDeclaration(node);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:codemod/codemod.dart';

import '../util.dart';
import 'boilerplate_utilities.dart';

/// Suggestor that updates props and state classes to new boilerplate.
Expand All @@ -29,15 +28,21 @@ import 'boilerplate_utilities.dart';
class SimplePropsAndStateClassMigrator extends GeneralizingAstVisitor
with AstVisitingSuggestorMixin
implements Suggestor {
final SemverHelper helper;

SimplePropsAndStateClassMigrator(this.helper);

@override
visitClassDeclaration(ClassDeclaration node) {
super.visitClassDeclaration(node);

if (!shouldMigrateSimplePropsAndStateClass(node)) return;
if (!shouldMigrateSimplePropsAndStateClass(node, helper)) return;

migrateClassToMixin(node, yieldPatch);
}
}

bool shouldMigrateSimplePropsAndStateClass(ClassDeclaration node) =>
shouldMigratePropsAndStateClass(node) && isSimplePropsOrStateClass(node);
bool shouldMigrateSimplePropsAndStateClass(
ClassDeclaration node, SemverHelper helper) =>
shouldMigratePropsAndStateClass(node, helper) &&
isSimplePropsOrStateClass(node);
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,21 @@ import 'package:over_react_codemod/src/boilerplate_suggestors/advanced_props_and
import 'package:over_react_codemod/src/boilerplate_suggestors/simple_props_and_state_class_migrator.dart';
import 'package:over_react_codemod/src/dart2_suggestors/props_and_state_companion_class_remover.dart';

import 'boilerplate_utilities.dart';

/// Suggestor that removes every companion class for props and state classes, as
/// they were only temporarily required for backwards-compatibility with Dart 1.
class StubbedPropsAndStateClassRemover
extends PropsAndStateCompanionClassRemover implements Suggestor {
final SemverHelper helper;

StubbedPropsAndStateClassRemover(this.helper);

@override
bool shouldRemoveCompanionClassFor(
ClassDeclaration candidate, CompilationUnit node) {
return super.shouldRemoveCompanionClassFor(candidate, node) &&
(shouldMigrateSimplePropsAndStateClass(candidate) ||
shouldMigrateAdvancedPropsAndStateClass(candidate));
(shouldMigrateSimplePropsAndStateClass(candidate, helper) ||
shouldMigrateAdvancedPropsAndStateClass(candidate, helper));
}
}
19 changes: 13 additions & 6 deletions lib/src/executables/boilerplate_upgrade.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import 'dart:convert';
import 'dart:io';

import 'package:codemod/codemod.dart';
Expand All @@ -21,6 +22,7 @@ import 'package:over_react_codemod/src/boilerplate_suggestors/simple_props_and_s
import 'package:over_react_codemod/src/boilerplate_suggestors/advanced_props_and_state_class_migrator.dart';
import 'package:over_react_codemod/src/boilerplate_suggestors/props_mixins_migrator.dart';
import 'package:over_react_codemod/src/boilerplate_suggestors/stubbed_props_and_state_class_remover.dart';
import 'package:over_react_codemod/src/boilerplate_suggestors/boilerplate_utilities.dart';
import 'package:over_react_codemod/src/ignoreable.dart';

const _changesRequiredOutput = """
Expand All @@ -31,14 +33,18 @@ const _changesRequiredOutput = """
Then, review the the changes, address any FIXMEs, and commit.
""";

void main(List<String> args) {
Future<void> main(List<String> args) async {
final query = FileQuery.dir(
pathFilter: (path) {
return isDartFile(path) && !isGeneratedDartFile(path);
},
recursive: true,
);

// TODO: determine file path of semver report
SemverHelper helper = SemverHelper(jsonDecode(
await File('lib/src/boilerplate_suggestors/report.json').readAsString()));

// General plan:
// - Things that need to be accomplished (very simplified)
// 1. Make props / state class a mixin
Expand Down Expand Up @@ -75,15 +81,16 @@ void main(List<String> args) {
// - If this is needed, it can be used for suggestors 3 and 4
//
//

exitCode = runInteractiveCodemodSequence(
query,
<Suggestor>[
StubbedPropsAndStateClassRemover(),
SimplePropsAndStateClassMigrator(),
AdvancedPropsAndStateClassMigrator(),
PropsMixinMigrator(),
StubbedPropsAndStateClassRemover(helper),
SimplePropsAndStateClassMigrator(helper),
AdvancedPropsAndStateClassMigrator(helper),
PropsMixinMigrator(helper),
PropsMetaMigrator(),
AnnotationsRemover(),
AnnotationsRemover(helper),
].map((s) => Ignoreable(s)),
args: args,
defaultYes: true,
Expand Down
111 changes: 111 additions & 0 deletions test/boilerplate_suggestors/boilerplate_utilities_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -267,5 +267,116 @@ main() {
});
});
});

group('getPublicExportLocations()', () {
SemverHelper helper;

group('returns null', () {
test('if there is no exports list', () {
helper = SemverHelper({'someOtherInfo': 'abc'});

getPublicExportLocationsTestHelper(
helper,
input: '''
@Props()
class _\$FooProps extends UiProps{
String foo;
int bar;
}
''',
expectedResult: null,
);
});

test('if props class is not in export list', () {
helper = SemverHelper({
'exports': {
'lib/web_skin_dart.dart/ButtonProps': {
'type': 'class',
'grammar': {
'name': 'ButtonProps',
'meta': ['@Props()']
}
}
}
});

getPublicExportLocationsTestHelper(
helper,
input: '''
@Props()
class _\$FooProps extends UiProps{
String foo;
int bar;
}
''',
expectedResult: null,
);
});
});

test('returns correct information if props class is in export list', () {
helper = SemverHelper({
"exports": {
"lib/web_skin_dart.dart/ButtonProps": {
"type": "class",
"grammar": {
"name": "ButtonProps",
"meta": ["@Props()"]
}
},
"lib/web_skin_dart.dart/FooProps": {
"type": "class",
"grammar": {
"name": "FooProps",
"meta": ["@Props()"]
}
},
"lib/web_skin_dart.dart/BarProps": {
"type": "class",
"grammar": {
"name": "BarProps",
"meta": ["@Props()"]
}
},
"lib/web_skin_dart.dart/DropdownSelectProps": {
"type": "class",
"grammar": {
"name": "DropdownSelectProps",
"meta": ["@Props()"]
}
}
}
});

getPublicExportLocationsTestHelper(
helper,
input: '''
@Props()
class DropdownSelectProps extends UiProps{
String foo;
int bar;
}
''',
expectedResult: {
"type": "class",
"grammar": {
"name": "DropdownSelectProps",
"meta": ["@Props()"]
}
},
);
});
});
});
}

void getPublicExportLocationsTestHelper(SemverHelper helper,
{String input, Map expectedResult}) {
CompilationUnit unit = parseString(content: input).unit;
expect(unit.declarations.whereType<ClassDeclaration>().length, 1);

unit.declarations.whereType<ClassDeclaration>().forEach((classNode) {
expect(helper.getPublicExportLocations(classNode), expectedResult);
});
}
18 changes: 18 additions & 0 deletions test/boilerplate_suggestors/report.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"exports": {
"lib/web_skin_dart.dart/ButtonProps": {
"type": "class",
"grammar": {
"name": "ButtonProps",
"meta": ["@Props()"]
}
},
"lib/web_skin_dart.dart/BarProps": {
"type": "class",
"grammar": {
"name": "BarProps",
"meta": ["@Props()"]
}
}
}
}
Loading