-
Notifications
You must be signed in to change notification settings - Fork 7
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
FED-570 Unify package rename codemod #251
Conversation
# Conflicts: # lib/src/executables/mui_migration.dart # lib/src/mui_suggestors/unused_wsd_import_remover.dart # lib/src/util/importer.dart
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was moved to the utils directory and generalized to take a package name argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same file that I moved from the unused_wsd_importer file in the mui_suggestors - I don't know why GitHub doesn't think so
ooo great catches @kealjones-wk - they were pretty straightforward fixes so I just went ahead and updated the codemod to address those! Thank you for catching those! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some #nits, really only one that would potentially be addressed.
- Changes look good
- Tests look good
- Worked well when I tested it locally
+10
import 'package:react_material_ui/react_material_ui.dart'; | ||
|
||
content() { | ||
// FIXME(unify_package_rename) Check what theme provider is wrapping this component: if it is a UnifyThemeProvider, remove this FIXME - no action is required; otherwise, migrate this component back to Web Skin Dart. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#nit it looks like this fixme insertion isn't idempotent, but the idempotency tests are passing because on the second run, the unify.
references are unresolved because the imports hadn't been updated.
I don't think this is worth fixing since it's so uncommon, but I thought I'd call it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's a good catch! Okay I'll keep that in mind and try not to re-run the codemod too much
// (which is more common for the WSD context), it fails here instead of failing the first test. | ||
setUpAll(resolvedContext.warmUpAnalysis); | ||
|
||
group('UnifyRenameSuggestor', () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#nit It makes me a bit nervous that we're not testing the import renaming and the usage renaming together, to ensure they're in sync with what they update.
But, I don't think we have an easy way of testing codemod sequences, and I don't think it'd be worth trying to set that up.
Perhaps if they were updated within the same suggestor, then that'd enable easier testing. But again, I'm not sure refactoring it would be worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+10
e1ff537
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+10 refresh
Merging once CI passes @Workiva/release-management-p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing... might not matter...
mainLibraryUnitResult.unit, mainLibraryUnitResult.lineInfo); | ||
yield Patch( | ||
insertInfo.leadingNewlines + | ||
"import '${importInfo.uri}'${importInfo.namespace != null ? ' as ${importInfo.namespace}' : ''};" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just throwing this out there that this doesnt seem to retain show
statements
example:
import 'package:react_material_ui/styles/theme_provider.dart' as mui_theme show UnifyThemeProvider;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh good catch! Yeah I think it'd be worth updating so this retains show
and hide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from RM
Motivation
Codemod to migrate react_material_ui usages to unify_ui.
Changes
The codemod goes through the following process:
UnifyRenameSuggestor
:mui
namespace usages tounify
(also updates the alpha versions)Release Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
dart pub global activate -s path ../over_react_codemod
dart pub global run over_react_codemod:unify_package_rename --yes-to-all
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: