CPLAT-9677 Improve class migration short-circuit logic + consumer communication #82

aaronlademann-wf · 2020-02-19T00:45:39Z

NOTE: I'd recommend just reading through the advanced_props_and_state_migrator_test.dart file without attempting to make sense of the diff.

Motivation

Currently, when the codemod runs the AdvancedPropsAndStateClassMigrator, the migrator does not short-circuit if a custom superclass, or one or more mixins is not present in the map of "known converted classes" that the migrators update as classes are converted.

This is done because we have no way to control the order in which classes are visited by the suggestor. This means that just because a superclass or mixin is not present in the map of "known converted classes" at the time the subclass is visited - does not mean it will never be converted.

We also need to improve how we communicate to the consumer when our codemod short-circuits because of public API detection / superclasses from external libs.

Changes

Expand the role of the ClassToMixinConverter class to record visits, as well as record whether the class that was visited was converted.
1. This allows us to short-circuit other migrations when a class that another class extends from / mixes in:
  1. Was visited, but not converted
  2. Was never visited (meaning it lives in an external lib)
Run the AdvancedPropsAndStateClassMigrator twice during the migration.
1. On the first run, short-circuit - but do not add FIXME comments when an unknown/unconverted superclass or mixin is encountered.
2. On the second run, short-circuit AND add FIXME comments when an unknown/unconverted superclass or mixin is encountered.
Add a --convert-classes-with-external-superclasses flag that can be set to force the conversion of classes that extend from or mix in classes that do not live in the same library.
Add a MigrationDecision class that is responsible for determining when short-circuits occur during migrations, as well as storing a reason comment that we will apply to keep the consumers informed about what they can do to either migrate the classes manually, or to make some adjustments before running the migration script again.
Move the StubbedPropsAndStateClassRemover so that it runs with the knowledge of which classes have been converted.
Misc. logic updates to handle additional edge cases while testing the migrators in graph_ui.
Add crap-tons of tests.

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review: @joebingham-wk @sydneyjodon-wk @greglittlefield-wf

QA Checklist

Tests were updated and provide good coverage of the changeset and other affected code
Manual testing was performed if needed
- Steps from PR author:
- Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

A Client Platform member has reviewed these changes
There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
For release PRs - Version metadata in Rosie comment is correct

@props

+ The advanced class migrator shouldn’t be removing @props() / @AbstractProps() annotations - we have another migrator for that. The removal of these annotations was causing the “companion” classes to not be removed when the class first had its annotation removed. + Also, the annotations should remain on the concrete class - not moved to the mixin

# Conflicts: # lib/src/boilerplate_suggestors/boilerplate_utilities.dart # lib/src/executables/boilerplate_upgrade.dart # test/boilerplate_suggestors/advanced_props_and_state_class_migrator_test.dart # test/boilerplate_suggestors/simple_props_and_state_class_migrator_test.dart # test/boilerplate_suggestors/stubbed_props_and_state_class_remover_test.dart

aviary-wf · 2020-02-19T00:47:35Z

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.

+ This metadata will get removed by the AnnotationsRemover anyway, so there is no reason to add the complexity here.

+ This allows us to operate on newly created mixins when migrators run after their creation. + Specifically, this is needed to ensure that props/state annotations are removed from newly created mixins by the AnnotationsRemover migrator.

joebingham-wk

Got through most of the changes, but it's dense so I thought I'd just send out what I had and finish up the review after lunch!

Overall, I like the new architecture for comments and keeping track of migrating decisions. I think it works well and makes it clear (or as clear as I can imagine) when comments should be posted. So that's awesome and I don't have any feedback on changes to how that works.

Otherwise just found some nits and small things!

lib/src/boilerplate_suggestors/advanced_props_and_state_class_migrator.dart

lib/src/boilerplate_suggestors/boilerplate_utilities.dart

lib/src/boilerplate_suggestors/advanced_props_and_state_class_migrator.dart

lib/src/boilerplate_suggestors/boilerplate_utilities.dart

test/boilerplate_suggestors/advanced_props_and_state_class_migrator_test.dart

greglittlefield-wf

Forgot to post this when I started reviewing a couple days ago 🤦‍♂

Starting another pass now...

lib/src/boilerplate_suggestors/boilerplate_utilities.dart

greglittlefield-wf

Final pass!

lib/src/boilerplate_suggestors/advanced_props_and_state_class_migrator.dart

test/boilerplate_suggestors/advanced_props_and_state_class_migrator_test.dart

greglittlefield-wf

+10

greglittlefield-wf · 2020-03-02T17:40:41Z

@rmconsole-wf

greglittlefield-wf · 2020-03-02T17:42:04Z

@Workiva/release-management-p

aaronlademann-wf added 6 commits February 18, 2020 12:41

Improve class migration short-circuits and FIXME comments

40655a8

DRY up the advanced boilerplate migrator tests

2e88fa3

Fix RTE

9e783c2

Harden annotation remover migrator logic

5a4472c

rmconsole-wf added the Merge Requirements Unmet label Feb 19, 2020

aaronlademann-wf added 13 commits February 19, 2020 08:05

Add clarifying comment

7232dc2

Re-use reservedBaesClassNames map

e9cf5f2

Clean up ClassToMixinConverter

d6a906b

Record visits of props/state mixins

5e7778d

Do not remove metadata from newly created mixins

eea9c4e

+ This metadata will get removed by the AnnotationsRemover anyway, so there is no reason to add the complexity here.

Add isBoilerplateCompatible method

7affd6d

+ This allows us to operate on newly created mixins when migrators run after their creation. + Specifically, this is needed to ensure that props/state annotations are removed from newly created mixins by the AnnotationsRemover migrator.

Beef up tests for PropsMetaMigrator

549e352

dartfmt is an embarrassment

2a6bd7a

Add moar logic / tests for abstract classes

51639c5

Fix dupe FIXME comment prevention logic

8710841

Add logic/tests for abstract class edge cases

472b51c

More comment removal logic hardening

661813f

Consider reserved mixins non-external

b98eb6b

aaronlademann-wf changed the title ~~[wip] CPLAT-9677~~ CPLAT-9677 Improve class migration short-circuit logic + consumer communication Feb 19, 2020

aaronlademann-wf marked this pull request as ready for review February 19, 2020 22:44

aaronlademann-wf mentioned this pull request Feb 19, 2020

CPLAT-9576 Consume SemverHelper #80

Merged

7 tasks

joebingham-wk reviewed Feb 20, 2020

View reviewed changes

sydneyjodon-wk reviewed Feb 21, 2020

View reviewed changes

test/boilerplate_suggestors/advanced_props_and_state_class_migrator_test.dart Show resolved Hide resolved

test/boilerplate_suggestors/advanced_props_and_state_class_migrator_test.dart Show resolved Hide resolved

greglittlefield-wf reviewed Feb 28, 2020

View reviewed changes

lib/src/boilerplate_suggestors/boilerplate_utilities.dart Show resolved Hide resolved

lib/src/boilerplate_suggestors/boilerplate_utilities.dart Outdated Show resolved Hide resolved

greglittlefield-wf requested changes Feb 28, 2020

View reviewed changes

aaronlademann-wf added 2 commits February 28, 2020 14:39

Don’t add FIXME when sub and super are abstract

13070df

Address CR feedback

72155d4

greglittlefield-wf approved these changes Mar 2, 2020

View reviewed changes

greglittlefield-wf merged commit cda95df into CPLAT-9205-new-boilerplate-codemod Mar 2, 2020

sydneyjodon-wk mentioned this pull request Mar 4, 2020

CPLAT-9205 Codemod for New Boilerplate #72

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPLAT-9677 Improve class migration short-circuit logic + consumer communication #82

CPLAT-9677 Improve class migration short-circuit logic + consumer communication #82

aaronlademann-wf commented Feb 19, 2020 •

edited

Loading

aviary-wf commented Feb 19, 2020

joebingham-wk left a comment •

edited

Loading

greglittlefield-wf left a comment

greglittlefield-wf left a comment

greglittlefield-wf left a comment

greglittlefield-wf commented Mar 2, 2020

greglittlefield-wf commented Mar 2, 2020

CPLAT-9677 Improve class migration short-circuit logic + consumer communication #82

CPLAT-9677 Improve class migration short-circuit logic + consumer communication #82

Conversation

aaronlademann-wf commented Feb 19, 2020 • edited Loading

Motivation

Changes

Review

QA Checklist

Merge Checklist

aviary-wf commented Feb 19, 2020

Security Insights

Action Items

joebingham-wk left a comment • edited Loading

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Mar 2, 2020

greglittlefield-wf commented Mar 2, 2020

aaronlademann-wf commented Feb 19, 2020 •

edited

Loading

joebingham-wk left a comment •

edited

Loading