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.

Sign up for GitHub

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

Jump to bottom

Migrate widget field when convert Stateless-based and Stateful-based to each other. #3275

Merged
merged 14 commits into from
Feb 3, 2024
Merged

Migrate widget field when convert Stateless-based and Stateful-based to each other. #3275

merged 14 commits into from
Feb 3, 2024

Conversation

Kurogoma4D
Copy link
Sponsor Contributor

@Kurogoma4D Kurogoma4D commented Jan 14, 2024 •
edited
Loading

In the official Dart analysis_server , when converting between Flutter's StatefulWidget and StatelessWidget, the widget field used in StatefulWidget is automatically added or removed.
However, this was not supported in the Assists within riverpod_lint.

In this PR, I have improved it to include the migration of that field in the conversion.

@codecov Codecov
Copy link

codecov bot commented Jan 14, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4621be5) 95.16% compared to head (b64259c) 95.16%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3275   +/-   ##
=======================================
  Coverage   95.16%   95.16%           
=======================================
  Files          53       53           
  Lines        2298     2298           
=======================================
  Hits         2187     2187           
  Misses        111      111

rrousselGit
rrousselGit reviewed Jan 15, 2024
View reviewed changes
packages/riverpod_lint_flutter_test/test/assists/convert_to_widget/convert_to_widget.dart Outdated
@@ -3,11 +3,22 @@ import 'package:hooks_riverpod/hooks_riverpod.dart';
import 'package:flutter_hooks/flutter_hooks.dart';

class Stateless extends StatelessWidget {
const Stateless({super.key});
const Stateless({
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than editing existing scenarios, could you add new ones at the end of the file? This would make the diff in golden tests much simplers to read.

At the moment, I wouldn't know if there's a regression.

Copy link
Sponsor Contributor Author

@Kurogoma4D Kurogoma4D Jan 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated test.
I checked the existing case for changes and noticed that the extra blank line at the end was removed when converting from Stateful-based Widget to Stateless-based Widget.
I thought this difference was fine, but how do you think?

before after
before after

@rrousselGit
Copy link
Owner

Lovely PR! This will definitely help folks a bunch :)

rrousselGit
rrousselGit reviewed Jan 22, 2024
View reviewed changes
...es/riverpod_lint_flutter_test/test/assists/convert_to_widget/convert_to_consumer_widget.json Outdated
Comment on lines 102 to 104
"offset": 1167,
"length": 0,
"replacement": ""
Copy link
Owner

Choose a reason for hiding this comment

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

Sounds incorrect. We're now replacing nothing with nothing

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.

Fixed in d5b8b6d .

@rrousselGit
Copy link
Owner

I've updated how goldens are formatted. It should be clearer to spot differences.
There seems to be more changes than just the newline thing you mentioned.

Ideally this should only add new lines to goldens. It shouldn't edit existing lines.
If there's a change to existing content, I'd need an explanation about why that change.

Also, could you update the changelog?

@Kurogoma4D
Copy link
Sponsor Contributor Author

@rrousselGit
Makes sense.
However, I expected there would be a difference in the conversion from Stateful-based to Stateless-based.

First, the previous method takes following steps:

  1. Copy the contents of the build method.
  2. Insert it into a StatefulWidget .
  3. Remove the State class.

Now, considering deleting the reference of the widget fields, we would get an error because it conflicts with step 3.
Then I changed these:

  1. Move the code between StatefulWidget and State under State.
  2. Remove the right bracket of StatefulWidget to the left bracket of State.

Thus, even in existing cases, there will be a difference.
For any other differences (include this comment ) , I'll look further.

rrousselGit
rrousselGit reviewed Jan 29, 2024
View reviewed changes
packages/riverpod_lint/lib/src/assists/convert_to_stateless_base_widget.dart
@@ -147,89 +150,250 @@ class ConvertToStatelessBaseWidget extends RiverpodAssist {
.whereType<MethodDeclaration>()
.firstWhereOrNull((element) => element.name.lexeme == 'createState');
if (createStateMethod != null) {
builder.addDeletion(createStateMethod.sourceRange);
builder.addDeletion(createStateMethod.sourceRange.getExpanded(1));
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add a comment about what this is about? The line isn't very clear to me

rrousselGit
rrousselGit reviewed Jan 29, 2024
View reviewed changes
packages/riverpod_lint/lib/src/assists/convert_to_stateless_base_widget.dart
}
}

// Original implemenation in
Copy link
Owner

Choose a reason for hiding this comment

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

Do you mind extracting all analyzer code in a separate file, explicitly stated as such?

rrousselGit
rrousselGit reviewed Jan 29, 2024
View reviewed changes
packages/riverpod_lint/lib/src/assists/convert_to_stateless_base_widget.dart
);
}

// ignore: prefer_foreach
Copy link
Owner

Choose a reason for hiding this comment

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

Don't ignore the lint :)

rrousselGit
rrousselGit reviewed Jan 29, 2024
View reviewed changes
packages/riverpod_lint/lib/src/assists/convert_to_stateless_base_widget.dart
}

// Search for the associated State class
final stateClass = findStateClass(widgetClass);
if (stateClass == null) return;

final fieldFinder = _FieldFinder();
Copy link
Owner

Choose a reason for hiding this comment

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

It would likely be worth extracting the field conversion logic to a method

@rrousselGit
Copy link
Owner

Thanks for your response!

I'll try running your PR locally to better understand the goldens diffs. At the moment it's a bit hard to understand why we have differences :)

rrousselGit
rrousselGit previously approved these changes Feb 2, 2024
View reviewed changes
Copy link
Owner

@rrousselGit rrousselGit left a comment

Choose a reason for hiding this comment

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

I'm approving this for now, but probably will merge at a later date.

I'd like to maybe change how goldens are stored. I don't like that I need to manually run the tests to understand the diffs

I'll try computing the actual resulting code, rather than the changes.

@rrousselGit
Copy link
Owner

Found a neat way to execute "changes" on a file. I'll update goldens accordingly, and check again your PR

@rrousselGit
Copy link
Owner

Alright, that's wayyy more readable now. Let's have a second look!

@rrousselGit rrousselGit merged commit 07d4b86 into rrousselGit:master Feb 3, 2024
40 of 44 checks passed
@rrousselGit
Copy link
Owner

Alright, I've looked at all the diffs, and everything looks great! Thanks again for this :)

@Kurogoma4D
Copy link
Sponsor Contributor Author

Kurogoma4D commented Feb 3, 2024 •
edited
Loading

Sorry for the late response, and thanks to the detailed confirmation :)
(in fact, using diff file for goldens result is neat.)

I haven't yet changes that you reviewed points, should I do it later?

@rrousselGit
Copy link
Owner

I haven't yet changes that you reviewed points, should I do it later?

I figured it wasn't worth delaying such a change for these small code improvements.
But if you want to do those changes, that'd be lovely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rrousselGit rrousselGit rrousselGit left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Kurogoma4D @rrousselGit