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 dart fix support for moved classes #45393

Closed
Piinks opened this issue Mar 19, 2021 · 7 comments
Closed

Add dart fix support for moved classes #45393

Piinks opened this issue Mar 19, 2021 · 7 comments
Assignees
Labels
analyzer-dartfix Issues with the dartfix package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on

Comments

@Piinks
Copy link
Contributor

Piinks commented Mar 19, 2021

We would like to supply fixes that can change a class name based on the method being called.

For example:

Scaffold.of(context).showSnackBar(SnackBar(content: Text('Snack')));

should be migrated to

ScaffoldMessenger.of(context).showSnackBar(SnackBar(content: Text('Snack')));

The is not specific to the same method being moved across classes, as we would want to be able to identify and compose fixes that can feasibly migrate classA.methodA to classB.methodB, or constructors too.

Another example of this would be going from this:

BlackListingTextInputFormatter()

to this:

FilteringTextInputFormatter.deny()

Not sure if that example should be a different use case, if so we can break it out into a separate issue.

@vsmenon vsmenon added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Mar 21, 2021
@srawlins srawlins added analyzer-dartfix Issues with the dartfix package P2 A bug or feature request we're likely to work on labels Mar 22, 2021
@craiglabenz
Copy link

I second this feature request, as it would open up a lot of very powerful migrations.

@bwilkerson
Copy link
Member

I think the following new kind of change would satisfy the requirements expressed here:


replacedBy

A replaced by change indicates that the specified element was replaced by another element. It has two keys: kind, and newElement.

The newElement key is required. The value of the newElement key is an element.


I think this would allow you to specify that the static method of in the class Scaffold was replaced by the static method of in the class ScaffoldMessenger, or that the constructor BlackListingTextInputFormatter was replaced by the constructor FilteringTextInputFormatter.deny.

Can you think of any other information you might want to have supported by such a change?

@craiglabenz
Copy link

As a Dartfix beginner, I'm going to ask a beginner question:

Could this be combined with Dartfixes for any parameters that were renamed between the old and new methods / constructors?

AKA, turning this:

ClassA({required int count})

to this

ClassB({required int limit})

@bwilkerson
Copy link
Member

Good question. I believe that the answer ought to be 'yes', that it ought to be possible to specify changes to the parameters just like you can with a rename change. But it's possible that there are implementation problems that I'm not currently aware of that would prevent that for some reason, so I'm hesitant to promise that it will be possible.

@pq
Copy link
Member

pq commented Oct 21, 2021

See also: https://dart-review.googlesource.com/c/sdk/+/214068 for a place where replacedBy could come in handy in migrated some SDK constant references. (There we'd like to change references to top-level variables (like READ) to class constants (like FileMode.read).)

/fyi @natebosch

@bwilkerson
Copy link
Member

I have committed https://dart-review.googlesource.com/c/sdk/+/221900, which includes a limited version of a replaceBy change. I believe that what's currently implemented is sufficient for both the use cases above and for the SDK use case. I'll update the documentation to include the new change, hopefully today.

I'm going to close this issue in preference of having new issues for any enhancement requests.

@pq
Copy link
Member

pq commented Dec 2, 2021

@bwilkerson: maybe cc @natebosch when you update the docs since this ought to get them unblocked?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-dartfix Issues with the dartfix package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

6 participants