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

[feature request] Extract class to a new file #30310

Open
Tracked by #55825
JFixby opened this issue Aug 1, 2017 · 57 comments
Open
Tracked by #55825

[feature request] Extract class to a new file #30310

JFixby opened this issue Aug 1, 2017 · 57 comments
Labels
analyzer-refactoring analyzer-server 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 type-enhancement A request for a change that isn't a bug

Comments

@JFixby
Copy link

JFixby commented Aug 1, 2017

image

@bwilkerson bwilkerson added analyzer-server 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 type-enhancement A request for a change that isn't a bug labels Aug 1, 2017
@scullabyte
Copy link

Any update on this? Possibly a way to do this with another application that will update all references? Thanks!

@cnsumner
Copy link

This would be extremely useful in VSCode. I have to do this operation manually 10s of times while mocking out an app UI.

@nilsreichardt
Copy link
Contributor

Any updates?

@bwilkerson
Copy link
Member

Unfortunately, no. I think there's a lot of value to this and similar refactorings that aid in code re-organization, but this work hasn't yet been prioritized high enough to be scheduled.

@magnussp
Copy link

Would love to see this feature added. Makes life so much easier.

@ajaygautam
Copy link

ajaygautam commented Nov 24, 2020

This has 86 likes now... in case you are using that as a voting system. (PS: As of today... this is the 10th highest liked issues)

@bwilkerson code re-organization is a programmer productivity task. I would very much love to see things that will make me more productive. Hence I disagree on your lack of value stance. Imagine the combined effort spent in manual refactoring!

@keehoo
Copy link

keehoo commented Jan 30, 2021

almost 4 years... geez..

@dkbast
Copy link

dkbast commented Feb 15, 2021

Looks like Jetbrains has this in Rider for .net ... I keep ending up on this site every few months when looking for this feature so I thought I'd share it here:
https://www.jetbrains.com/help/rider/Refactorings__Move__Type_to_Another_File.html

@dfaveur
Copy link

dfaveur commented Feb 25, 2021

Yes please. It is very cumbersome currently to isolate widget per file

@krema
Copy link

krema commented Mar 13, 2021

I just discovered a plugin that we can use in the meantime at least for flutter.

https://github.com/marius-h/flutter_enhancement_suite

Screen Shot 2021-03-13 at 14 36 32 PM

@ghost
Copy link

ghost commented Jul 27, 2021

Unluckily such extension doesn't work on VS code.

@asashour

This comment was marked as outdated.

@mrRedSun
Copy link

mrRedSun commented May 4, 2022

Why isn't this done yet?

@srawlins
Copy link
Member

@scheglov can you confirm this has been implemented?

@scheglov
Copy link
Contributor

scheglov commented Jul 20, 2022

Sorry, closed a wrong issue.

@scheglov scheglov reopened this Jul 20, 2022
@scheglov
Copy link
Contributor

@srawlins I was talking only about extensions #38894, nothing about this one :-)

@srawlins
Copy link
Member

srawlins commented Jul 20, 2022

Ah excellent. Thanks!

@mannyvergel
Copy link

Belated happy 5th birthday to this issue 🥳

@dhxyz
Copy link

dhxyz commented Nov 6, 2022

Are there any updates on this? #1831 seems promising.

@cbenhagen
Copy link
Contributor

@DanTup do you know if this can already be tested in the Intellij plugin? Haven't found a related issue in the plugin repo.

@DanTup
Copy link
Collaborator

DanTup commented Mar 26, 2023

@cbenhagen as far as I know, the new refactor system is only currently available over LSP (the newer protocol used by VS Code).

For IntelliJ, it'll need adding to the original server protocol (and work in the plugin to handle the server asking for input like filenames), but I don't think work has started on that yet.

@DanTup
Copy link
Collaborator

DanTup commented Mar 28, 2023

FYI, I'm making some slight changes to the opt-in flag for experimental refactors. The existing flag ("dart.experimentalNewRefactors") was for entirely enabling the new refactoring system. This was fine for initial development but doesn't allow us to have both experimental + stable refactors using this system in future.

In an upcoming release, that flag will be replaced by "dart.experimentalRefactors" (the word "New" has been removed) and it will now control only specific refactors that are marked as experimental.

For the time being, the "Move to File" refactor will continue to be considered experimental. You're welcome to try it out and provide feedback to help find any remaining issues, but be aware that in the current stable and current Dart 3 pre-release SDKs it's incomplete and has several known issues, such as:

  • file headers may be inserted after the moved class
  • moved classes may be inserted above import statements
  • moving sealed classes does not automatically move their direct subclasses
  • moving sealed classes/subclasses that are spread across part files is allowed and results in invalid code (for now, moving sealed types with direct subclasses in other parts will not be supported)

If you find any issues not listed above, please file new issues (and feel free to cc me). If you want to update your settings in advance so the experimental refactor doesn't disappear when you get an SDK with this change, you can include both flags in your VS Code settings:

	"dart.experimentalNewRefactors": true, // current flag
	"dart.experimentalRefactors": true, // future flag

Though be aware that these flags may also cause other incomplete refactors to show up in future, so you may wish to disable them once this refactor is no longer considered experimental.

@dhxyz
Copy link

dhxyz commented Apr 14, 2023

@DanTup:

Thank you so much for implementing this feature. I have been using it quite extensively in the meantime and it has saved me a lot of time.

I would like to propose something for the imports, though. Instead of, for example, importing material.dart, the feature imports material widgets selectively, like so:

import 'package:flutter/src/material/icons.dart';
import 'package:flutter/src/material/page.dart';
import 'package:flutter/src/painting/border_radius.dart';
import 'package:flutter/src/painting/box_border.dart';
import 'package:flutter/src/painting/box_decoration.dart';
import 'package:flutter/src/painting/edge_insets.dart';
import 'package:flutter/src/rendering/box.dart';
import 'package:flutter/src/rendering/flex.dart';
import 'package:flutter/src/widgets/basic.dart';
import 'package:flutter/src/widgets/container.dart';
import 'package:flutter/src/widgets/framework.dart';
import 'package:flutter/src/widgets/icon.dart';
import 'package:flutter/src/widgets/navigator.dart';
import 'package:flutter/src/widgets/text.dart';

To prevent this, I have always deleted all of the automatic imports and then I just pasted all of the imports of the source file at the top of the new file. Then, I hit save, which triggers "fix all" through codeActionsOnSave. This removes all unused imports.

Please bear in mind that I am a total noob in asking this, but: Wouldn't it be easier and better if experimentalRefactors did the same thing as I do manually (pasting all imports from the source file & fix all) instead of looking for every widget selectively (if that even is what it does)?

I am still on Dart 2.19.6, though. So, this might not even be relevant anymore. 😅 Thanks, again. It's an awesome feature.

@DanTup
Copy link
Collaborator

DanTup commented Apr 14, 2023

@dhxyz FWIW it's discouraged to import from another packages src folder - see https://dart.dev/guides/language/effective-dart/usage#dont-import-libraries-that-are-inside-the-src-directory-of-another-package.

Wouldn't it be easier and better if experimentalRefactors did the same thing as I do manually (pasting all imports from the source file & fix all) instead of looking for every widget selectively (if that even is what it does)?

If I understand correctly, the code essentially does an improved version of this. It locates all of the relevant imports and then re-imports them into the destination file (with the same prefixes if applicable).

However, when imports are added there is some code that tries to "improve" imports if they are in a src folder that you shouldn't be importing from (this fixes issues like #49081). I guess there's a question here of whether that code should be running in this case.

@bwilkerson do you have an opinion on what we should do if the source file contains src imports? One one hand this seems desired (we don't want to add src imports where they shouldn't be used), but on the other hand if a user already had these imports they might expect it to copy them.

@dhxyz
Copy link

dhxyz commented Apr 14, 2023

do you have an opinion on what we should do if the source file contains src imports?

About that - this is not what is happening in my case. Not sure if I expressed that correctly, sorry for that. The above example is what the feature does when the source file (by that I mean the file where the widget I want to refactor is from) contains 'package:flutter/material.dart' as an import. There were no src imports in the source file.

@DanTup
Copy link
Collaborator

DanTup commented Apr 14, 2023

@dhxyz oh sorry, I mis-read what you wrote. If it's adding package:flutter/src imports that weren't in your source file that's definitely a bug (and sounds like it's not using the code I blamed above). I'm out today, but will check if this still occurs on the latest code when I'm back next week. Thanks!

@bwilkerson
Copy link
Member

I agree, that's definitely a bug. Thanks for bringing this to our attention so that we can get it fixed before shipping the refactoring.

@babbage
Copy link

babbage commented Apr 27, 2023

This is great, most useful. I likewise am seeing the bug reported above where imports from the flutter framework are over-specific instead of just import 'package:flutter/material.dart';

I also noted another issue with our code I wanted to report. In the original file:
import 'dart:developer' as developer;

In the new file from the refactoring:
import 'dart:developer';

As a result, calls in the new file to developer.log(...) are not resolved correctly and are marked as errors.

@DanTup
Copy link
Collaborator

DanTup commented Apr 27, 2023

@babbage I can reproduce that (it specifically seems to be that prefixed functions are not handled) and have a fix in progress. Thanks!

copybara-service bot pushed a commit that referenced this issue May 15, 2023
See #30310 (comment).

Change-Id: I26c2ff905a474d24b85ca970dda0f67398ca7adb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/298822
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
copybara-service bot pushed a commit that referenced this issue May 15, 2023
…op-levels to new files

Previously we would just import the library containing the declaration (which for other packages could be in `src/`).

See #30310 (comment).

Change-Id: I1f2c8f480b60240bd7f88e037fd768e157f96f17
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/301400
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@mihalycsaba
Copy link

Enabled it and it works well enough, it's slower than other refactor actions, takes like 2 seconds, but it's okay better than doing this manually.

One suggestion for improvement if it's possible. When the code was moved to a new file, the original file still retained the imports that were only used by that code. It would be nice if it removed the imports that are no longer needed.

@bwilkerson
Copy link
Member

We'll have to look into the performance issue to see whether it's possible to improve the situation, but thanks for letting us know.

Yes, not removing unused imports from the original file is a known issue. I agree it would be nice for the refactoring to do that for you. We decided to ship without it because we have a separate fix to remove unused imports (a workaround) and thought it was better to get the refactoring out to users sooner.

@mihalycsaba
Copy link

I think this should work for functions too.

@bwilkerson
Copy link
Member

As it's currently implemented, it ought to work for any set of contiguous non-private top-level declarations (though there might be a couple of exceptions I'm forgetting about). If it isn't working for some particular case, please open a new issue and provide a reproduction case.

@RyanParkGH
Copy link

@bwilkerson Is it possible to move to an existing file without it being overwritten? I'd like to consolidate Classes and Methods into an existing file but it appears that the file will always be overwritten on each refactor. Am I missing something?

@DanTup
Copy link
Collaborator

DanTup commented Jan 24, 2024

Moving to an existing file is supported.. however I just tested it and see that when I select an existing file VS Code gives this message:

image

This message is misleading, and we actually do not replace the file, we merge into it. It seems like it's because of the VS Code API we use to select a file (showSaveDialog). I'm not sure if it's always done and I missed it, or it's new.

I've opened microsoft/vscode#203326 in VS Code to see if there could be a way to suppress this (or have an API for selecting files that does not do this).

@RyanParkGH
Copy link

@DanTup thanks for the insight! I stopped dead in my tracks after receiving the new, rather, scary looking warning in VS Code:

image

I'll have to give this a go myself, it'll be very useful indeed!

@DanTup
Copy link
Collaborator

DanTup commented Jan 24, 2024

@Autom8r-82 oof, that one is even worse (it more explicitly states that something bad will happen that will not). Thanks for the heads up - I'll see if I can find any other options for this.

@FMorschel
Copy link
Contributor

Just to point out the importing of necessary libraries could be handled by #30296.

@bwilkerson
Copy link
Member

I was under the impression that imports are correctly being computed. Is that not the case?

@FMorschel
Copy link
Contributor

FMorschel commented Jun 5, 2024

I missed that there was an existing flag to enable it. Sorry 🙃! Figured it was still unavailable.

@DanTup
Copy link
Collaborator

DanTup commented Jun 6, 2024

I missed that there was an existing flag to enable it

A flag to enable what? My understanding is that (for VS Code) this refactor is available on the latest SDKs and will add any necessary imports. No flags should be required (since v3.72.0 of Dart-Code was released and the stable SDK available at the time). If that's not what you're seeing, please let me know / file an issue.

(there are some issues with the dialog showing an overwrite prompt noted above which are not resolved yet though)

@FMorschel
Copy link
Contributor

FMorschel commented Jun 6, 2024

Here you said that this was an experimental feature

FYI, I'm making some slight changes to the opt-in flag for experimental refactors. The existing flag ("dart.experimentalNewRefactors") was for entirely enabling the new refactoring system. This was fine for initial development but doesn't allow us to have both experimental + stable refactors using this system in future.

In an upcoming release, that flag will be replaced by "dart.experimentalRefactors" (the word "New" has been removed) and it will now control only specific refactors that are marked as experimental.

For the time being, the "Move to File" refactor will continue to be considered experimental. You're welcome to try it out and provide feedback to help find any remaining issues, but be aware that in the current stable and current Dart 3 pre-release SDKs it's incomplete and has several known issues, such as:

* file headers may be inserted after the moved class

* moved classes may be inserted above import statements

* moving sealed classes does not automatically move their direct subclasses

* moving sealed classes/subclasses that are spread across `part` files is allowed and results in invalid code (for now, moving sealed types with direct subclasses in other `part`s will not be supported)

If you find any issues not listed above, please file new issues (and feel free to cc me). If you want to update your settings in advance so the experimental refactor doesn't disappear when you get an SDK with this change, you can include both flags in your VS Code settings:

	"dart.experimentalNewRefactors": true, // current flag
	"dart.experimentalRefactors": true, // future flag

Though be aware that these flags may also cause other incomplete refactors to show up in future, so you may wish to disable them once this refactor is no longer considered experimental.

I have not seen any indications (or I missed it completely) on any comments after that explaining it was available.

I'm sorry, I've not seen that release post of the extension. Found out about this site not long ago. But please enlighten me on why is this issue still open then. I came here and figured it was still under development because it was opened.

Edit

It is only because of those issues with the dialogues?

@DanTup
Copy link
Collaborator

DanTup commented Jun 6, 2024

@FMorschel sorry for the confusion, you're right that I didn't post in this issue when this refactor was marked as not-experimental (which I did in a3399e3), only in the Dart-Code issue/release notes. I certainly should've noted it here.

Currently this refactor is only available in VS Code and not in IntelliJ/Android Studio. Making it available to IntelliJ/AS will require work in their Dart plugin, but might also require some additional work here to allow them to call the new refactoring system (possibly through LSP).

@bwilkerson
Copy link
Member

@jwren

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-refactoring analyzer-server 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 type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests