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

Add conversion assists for various widgets, including Hook-based widgets #2306

Merged
merged 30 commits into from Apr 8, 2023

Conversation

K9i-0
Copy link
Sponsor Contributor

@K9i-0 K9i-0 commented Mar 12, 2023

Since ConvertToConsumerWidget and ConvertToConsumerStatefulWidget are useful, I've added conversions to other Widgets.

List of assists I've added:

  • ConvertToHookWidget
  • ConvertToStatefulHookWidget
  • ConvertToHookConsumerWidget
  • ConvertToStatefulHookConsumerWidget
  • ConvertToStatelessWidget
  • ConvertToStatefulWidget

Priority of Assists

  • ConvertToHookConsumerWidget
  • ConvertToHookWidget
  • ConvertToConsumerWidget
  • ConvertToStatelessWidget
  • ConvertToStatefulHookConsumerWidget
  • ConvertToStatefulHookWidget
  • ConvertToConsumerStatefulWidget
  • ConvertToStatefulWidget

Optional Assists

Display only when there is a dependency on hooks_riverpod

  • ConvertToHookConsumerWidget
  • ConvertToStatefulHookConsumerWidget

Display only when there is a dependency on flutter_hooks

  • ConvertToHookWidget
  • ConvertToStatefulHookWidget

@codecov
Copy link

codecov bot commented Mar 12, 2023

Codecov Report

Merging #2306 (b2c8fb9) into master (32742bc) will not change coverage.
The diff coverage is n/a.

❗ Current head b2c8fb9 differs from pull request most recent head 3975a36. Consider uploading reports for the commit 3975a36 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2306   +/-   ##
=======================================
  Coverage   95.25%   95.25%           
=======================================
  Files          53       53           
  Lines        2253     2253           
=======================================
  Hits         2146     2146           
  Misses        107      107           

@K9i-0 K9i-0 changed the title Add convert to widget assists Add conversion assists for various widgets, including Hook-based widgets Mar 17, 2023
@K9i-0 K9i-0 marked this pull request as ready for review March 21, 2023 12:35
),
),
hookConsumerWidget(
widgetName: 'HookConsumerWidget',
Copy link
Owner

Choose a reason for hiding this comment

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

I'd expect hook variants to have a higher priority than non-hook variants, but only show up if the analyzed package depends on hooks_riverpod/fluter_hooks.

Copy link
Owner

Choose a reason for hiding this comment

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

Bump on this, how that hooks are optional :)

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I tried to change the priority order as follows:

  1. ConvertToHookConsumerWidget
  2. ConvertToHookWidget
  3. ConvertToConsumerWidget
  4. ConvertToStatelessWidget
  5. ConvertToStatefulHookConsumerWidget
  6. ConvertToStatefulHookWidget
  7. ConvertToConsumerStatefulWidget
  8. ConvertToStatefulWidget

However, I'm not very confident about this order, so I'd appreciate it if you could let me know if this order seems good.

Additionally, I made some conversions optional.

Display only when there is a dependency on hooks_riverpod

  • ConvertToHookConsumerWidget
  • ConvertToStatefulHookConsumerWidget

Display only when there is a dependency on flutter_hooks

  • ConvertToHookWidget
  • ConvertToStatefulHookWidget

@rrousselGit
Copy link
Owner

Great! Thanks for the change :)
I'll review this more in depth a bit later.

@rrousselGit
Copy link
Owner

Sorry for the delay. I've updated custom_lint's context object to include the pubspec.

Would you be interested in updating these to only apply hooks assists if hooks_riverpod is in the pubspec's dependencies? It should be a simple if condition.

@K9i-0
Copy link
Sponsor Contributor Author

K9i-0 commented Apr 7, 2023

I'll give it a try👍

node,
resolver.source,
// This adjustment assumes that the priority of the standard "Convert to StatelessWidget" is 30.
isExactlyStatefulWidget ? -4 : 0,
Copy link
Owner

Choose a reason for hiding this comment

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

Use named parameters

Copy link
Owner

Choose a reason for hiding this comment

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

Also, what is this supposed to be doing?

Copy link
Sponsor Contributor Author

@K9i-0 K9i-0 Apr 8, 2023

Choose a reason for hiding this comment

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

This adjustment is due to the fact that the existing Convert to StatefulWidget and Convert to StatelessWidget both have a priority of 30.
https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/lib/src/services/correction/assist.dart#L224-L233

The priority of assists is generally set with Convert to StatefulWidget as 30, but the order will not be as expected when the source Widget is a StatefulWidget.

SCR-20230408-st6

By making an adjustment of -4 to the priority only when it's a StatefulWidget, the priority is based on Convert to StatelessWidget 30, resulting in the expected display.

Copy link
Owner

Choose a reason for hiding this comment

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

I see, thanks!

@rrousselGit
Copy link
Owner

This is looking pretty good! I'm currently having a second look at the code to make sure, but it's good so far :)

@rrousselGit rrousselGit merged commit 8808488 into rrousselGit:master Apr 8, 2023
25 checks passed
@rrousselGit
Copy link
Owner

Looking good! Thanks for your work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants