-
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-2994 Update class component defaults codemod to take public-ness into account #297
Conversation
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.
Just two comments!
lib/src/dart3_suggestors/null_safety_prep/utils/class_component_required_fields.dart
Show resolved
Hide resolved
exitCode = await runInteractiveCodemodSequence( | ||
dartPaths, | ||
[ | ||
ClassComponentRequiredDefaultPropsMigrator(null, recommender), |
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.
Does this not work when added to the sequence below, like so?
exitCode = await runInteractiveCodemodSequence(
dartPaths,
[
+ ClassComponentRequiredDefaultPropsMigrator(null, recommender),
RequiredPropsSuggestor(
recommender,
trustRequiredAnnotations:
parsedArgs[_Flags.trustRequiredAnnotations] as bool,
),
],
defaultYes: true,
args: codemodArgs,
additionalHelpOutput: argParser.usage,
);
Not a huge deal, but it would be nice if consumers who wanted to apply all patches via the prompt (like if they didn't think to pass --yes-to-all
) didn't have to do so twice.
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.
It combines the patches when I put them together 😢
'mixin TestPrivatePropsMixin on UiProps {\n'
' String/*?*/ notDefaultedOptional;\n'
' /*late*/ String notDefaultedAlwaysSet;\n'
' /*late/*?*/*/ String/*?*/ defaultedN/*?*/ullable;\n'
' /*late*/ num/*!*/ defaultedNonNullable;\n'
'}\n'
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.
Hm okay, thanks for trying!
There's probably a bug in dart_codemod, then, possibly due to reusing FileContext/AnalysisContextCollection between suggestors.
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
@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.
+1 from RM
Motivation
Right now, the class component codemod always makes defaulted props required, but that goes against our guidance (see here) in cases where those props are public.
This codemod should only make those props required when classes are private.
Changes
Release Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: