search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.23k stars 1.57k forks source link

Sealed sub-classes "Move to file" doesn't show #56391

Open FMorschel opened 2 months ago

FMorschel commented 2 months ago

Take this sample:

sealed class A {}

final class B extends A {}

image image

With this sample, I can get the quick-fix at the declaration of A (line 1 with the suggestion to move n declarations to file) but not at B (line 3). Is this intended?

CC: @DanTup

dart-github-bot commented 2 months ago

Summary: The "Move to file" quick-fix is available for sealed classes but not for their subclasses. The user expects the quick-fix to be available for both the sealed class and its subclasses.

bwilkerson commented 2 months ago

The refactoring isn't as complete as I'd like it to be.

We can't move either A or B in isolation (assuming that the destination isn't a part file in the same library, which we also ought to support). We made a decision to move subclasses of a sealed class with the sealed class, but hadn't really considered the other direction.

The one argument I can see against expanding the refactoring is as follows. By looking at A a user can see that it's sealed and if they understand the semantics will understand that subclasses need to move with it. By looking at B there's no indication that it's a subclass of a sealed class, so it might be surprising that the superclass and all of the siblings are also moved. There might be a way to overcome that possible confusion by changing the name of the refactoring or by some other means.

DanTup commented 2 months ago

One possibility (albeit slightly more involved) could be to prompt the user:

Selection includes subclasses of sealed classes that just remain in the same file, move the superclass and other subclasses too?

[Include related classes] [Cancel refactoring]

(the wording could be better 😄)

I think we have something similar for renames that produce warnings (where you can choose to "Refactor anyway" or cancel). It does rely on the client supporting showMessageRequest, but we could have the current behaviour if that's not the case.

FMorschel commented 2 months ago

assuming that the destination isn't a part file in the same library, which we also ought to support

Is there an issue for this feature already?

bwilkerson commented 2 months ago

Not as far as I can see, but my skills at searching the Github issue tracker aren't great.

bwilkerson commented 2 months ago

We have traditionally discouraged the use of part files for anything other than generated code, which is part of why that support doesn't exist. If enhanced parts are supported then such a feature might be higher priority than it is today.

FMorschel commented 2 months ago

Not as far as I can see, but my skills at searching the Github issue tracker aren't great.

Not just you, the search here is a bit weird hahaha.

We have traditionally discouraged the use of part files for anything other than generated code, which is part of why that support doesn't exist. If enhanced parts are supported then such a feature might be higher priority than it is today.

I see. Thanks for explaining.