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

Change file name when change class name #3354

Closed
jacobggman opened this issue May 19, 2021 · 12 comments
Closed

Change file name when change class name #3354

jacobggman opened this issue May 19, 2021 · 12 comments
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Milestone

Comments

@jacobggman
Copy link

Every time that I want to refactor a name of a class in my code I need to change the class name and also change the file name. I think this is a waste of time to change the name of the same thing twice, this is an another step to make refactor harder. For example, I have a class that have the name of NodeKey and I want to change the class name to FileKey I need to change the class name and the file name from node_key.dart to file_key.dart.

I thing that this process need to be automated and it will same time for many users and make refactoring easier. On Change Symble that change class name the extension will check if the file name have the same name as the changed class name (taking into consideration the name conventions of UpperCamelCase for class names and lowercase_with_underscores for file names) and if they have the same name change also the file name to the new name (with the name conventions).

Alternative solutions are: use existing extensions that does it, if you know any, or this is already possible in this extension and I don't know about it or make a new extension that does it.

I will be happy to implement this if it is possible :) If possible you know any file/files you think that I need to change to make this feature?

@DanTup
Copy link
Member

DanTup commented May 19, 2021

I think it's a fair request. There are two places it could be implemented:

  1. Directly in this extension, use LSP middleware to sniff when a rename happens and trigger the file rename
  2. In the LSP servers rename handler which could then include the rename as part of the WorkspaceEdit it returns

I think the second one makes most sense (cc @bwilkerson for his opinion) since other LSP clients would also benefit. In either case, I think there needs to be a setting to control this (for ex. a nullable boolean, where null means prompt the user, and true/false would just always do that - the prompt could include options for "this time only"/"always"/"never" which would set the option accordingly). We can prompt the user from the server using LSP's window/showMessageRequest.

@DanTup DanTup added in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server labels May 19, 2021
@DanTup DanTup added this to the On Deck milestone May 19, 2021
@jacobggman
Copy link
Author

I also think the second approach is better. From what I understand the extension is manage the saving and the passing of the parameters for the handler, so the dart code doesn't need to save the "user preferences". Am I right?

Also, I fork the dart SDK repo to make the changes and basically I finish writing the code but I haven't tested it yet. I am not sure how do I test if what I did is working. I tried to build the analysis_server project, but it depends on those projects:

analysis_server -> analyzer_plugin -> analyzer_utilities -> analyzer

And I can't build analyzer_utilities:

Because test <1.3.0 requires SDK version >=1.8.0 <2.0.0-∞ and test >=1.16.0-nullsafety <1.16.0-nullsafety.8 requires SDK version >=2.10.0-0 <2.12.0, test <1.3.0-∞ or >=1.16.0-nullsafety <1.16.0-nullsafety.8 is forbidden.
And because test >=1.16.0-nullsafety.8 <1.16.0-nullsafety.12 depends on yaml ^2.0.0 and test >=1.3.0 <1.16.0-nullsafety depends on yaml ^2.0.0, test <1.16.0-nullsafety.12 requires yaml ^2.0.0.
And because test >=1.16.0-nullsafety.10 <1.16.1 depends on analyzer >=0.36.0 <0.42.0 and test >=1.16.1 <1.16.6 depends on analyzer >=0.39.5 <2.0.0, test <1.16.6 requires yaml ^2.0.0 or analyzer from hosted.
And because every version of analyzer from path depends on yaml ^3.0.0 and test >=1.16.6 depends on analyzer from hosted, analyzer from path is incompatible with test.
So, because analyzer_utilities depends on both analyzer from path and test any, version solving failed.
exit code 1

Also can you take a look at my code and check if what I did it fine? I am not sure if the file paths will be working and the way I got them and not sure about the return of WorkspaceEdit, I need to return two WorkspaceEdits?

@DanTup
Copy link
Member

DanTup commented May 24, 2021

From what I understand the extension is manage the saving and the passing of the parameters for the handler, so the dart code doesn't need to save the "user preferences". Am I right?

That's a good question. It's correct that the server cannot modify the settings itself. In the extension (pre-LSP) this would be quite simple, but there's not a good way to do this with LSP. Implementing a custom message would work for VS Code, but not other editors so I'm not sure that's worth doing.

So for now, probably it would be better for the prompt to just ask if you want to rename the file with only options for Yes/No and the user would have to change the setting themselves (this could be mentioned in the prompt).

Also can you take a look at my code and check if what I did it fine?

Sure - it would be better to open a review to make it easier to comment (it'll need reviewing by a Googler there to merge anyway), but I've had a quick look through and made a few comments below.

  • There shouldn't be any changes to protocol_generated, instead the setting should be added to the client_configuration.dart file (see enableSdkFormatter for a an example, although that one is just a boolean whereas this one should be nullable)
    This should also be documented in https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/tool/lsp_spec/README.md#client-workspace-configuration so other LSP clients (or their users) can discover it (we'd also want to add it to Dart-Code's package.json so it shows up in the Settings UI)
  • Based on my comment above, I think we'd need to remove thisTimeOnly and just use Yes and No for the prompt instead
  • I would break up the code added to handler_rename into some private methods as it's grown quite large now (for example a method to prompt the user and collect the reply, and another to wrap it that first checks the filename before calling it)
  • I think the call to server.getResolvedUnit(oldPath) may be redundant, as this is already available in unit.result?
  • We might not need to call MoveFileRefactoring directly - I would have expected if we included a rename ResourceOperation in the original results (we'd need to check the client supports this using the clientCapabilities) the client should then fire the standard rename events (that will end up calling the willRenameFiles handler). If that doesn't seem to be the case, let me know and we can find out whether that's a bug or not.
  • It might be simpler to use regex to convert the class name to a filename, for ex. something like:
    final _upperCasePattern = RegExp('[A-Z]');
    String _toFileName(String className) => className.replaceAllMapped(_upperCasePattern, (match) => match.start == 0 ? match[0]! : '_${match[0]}').toLowerCase();
  • Tests will be needed - they'd go in lsp/rename_test.dart. There are some existing tests that use _test_rename_prompt to handle a prompt from the server and then check the results afterwards. There should be tests that check both the behaviour of Yes and No responses when the setting is set to prompt, and also that there is no prompt (and the right result is returned) if the setting is set to true/false. To set the config you can use the provideConfig function (see test_lineLength in test/lsp/format_test.dart for an example of using this).

If anything isn't clear or you have any more questions, let me know!

@DanTup DanTup modified the milestones: On Deck, v3.24.0 May 24, 2021
@DanTup
Copy link
Member

DanTup commented Jun 28, 2021

@jacobggman is this something you're still looking at (or planning to)? There's no rush to do so, but I'm happy to take a look instead if that suits you better. Thanks!

@DanTup DanTup modified the milestones: v3.24.0, On Deck, v3.25.0 Jun 28, 2021
@jacobggman
Copy link
Author

Hi! I have been a little bit busy lately. I will try to finish this issue until next week. Thanks!

@jacobggman
Copy link
Author

@DanTup I not finish yet this issue, but I want to open a review. But when I am typing git cl upload -s git saying git: 'cl' is not a git command. It is fine if I just open PR or that I need to download something?

@DanTup
Copy link
Member

DanTup commented Jul 7, 2021

@jacobggman The "git cl" command comes from depot_tools. The instructions for setting that up are in the Dependencies section here:

https://github.com/dart-lang/sdk/wiki/Building

It includes a binary git-cl - once that's in your PATH, the git cl command will use it.

@DanTup DanTup modified the milestones: v3.25.0, v3.26.0 Jul 20, 2021
@jacobggman
Copy link
Author

jacobggman commented Jul 26, 2021

I open a review of this issue, so now hopefully now it's will be easier for you to review my code.

I update the code, but I still not write tests because I did not understand this part completely:

  • We might not need to call MoveFileRefactoring directly - I would have expected if we included a rename ResourceOperation in the original results (we'd need to check the client supports this using the clientCapabilities) the client should then fire the standard rename events (that will end up calling the willRenameFiles handler). If that doesn't seem to be the case, let me know and we can find out whether that's a bug or not.

I did not manage to understand what is ResourceOperation. I need to add new rename method? Do I need to call it? I am little bit confused.

@DanTup
Copy link
Member

DanTup commented Aug 5, 2021

Sorry for the delay!

I did not manage to understand what is ResourceOperation. I need to add new rename method? Do I need to call it?

I think in your original code you were directly calling the MoveFileRefactoring refactor to generate the edits, but I was expecting the client could handle this. So in the WorkspaceEdit sent back, it would instead include a ResourceOperation.rename:

https://microsoft.github.io/language-server-protocol/specifications/specification-current/#workspaceEdit

I'm not certain if this would trigger VS Code to fire the onWillRename event, so we might need to do both - it would require some testing. If you haven't already set it up, you should be able to test in VS Code using the local analyzer by setting the dart.analyzerPath setting in VS Code, for example:

"dart.analyzerPath": "/Users/danny/Dev/dart-sdk/sdk/pkg/analysis_server/bin/server.dart",

@jacobggman
Copy link
Author

Hi. I am a little busy lately and I don't have enough time to finish this issue, if you want, you can take from where I stop. Thanks you for your time!

@DanTup DanTup modified the milestones: v3.26.0, On Deck Aug 24, 2021
@DanTup DanTup modified the milestones: On Deck, v3.27.0 Sep 2, 2021
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Sep 2, 2021
… LSP

Fixes Dart-Code/Dart-Code#3354.

Change-Id: I2a11a54e0a2c35d525ee08e0849e35c421f6e195
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/212291
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
@DanTup DanTup closed this as completed in f6f61f7 Sep 6, 2021
@DanTup
Copy link
Member

DanTup commented Sep 6, 2021

This works is complete but requires two things to use it:

  1. The next release of Dart-Code (likely early next month)
  2. An updated Dart SDK that includes dart-lang/sdk@3aa464a (I believe this will be v2.15, but it may be some way off release)

It's off by default but can be turned on with the dart.renameFilesWithClasses setting (which can be "never", "prompt" or "always").

If you want to try it before them, you can manually add the setting to your VS Code settings (ignore that VS Code says it doesn't exist):

"dart.renameFilesWithClasses": "prompt"

And use a bleeding edge SDK build (not recommended for general dev, stick to stable!):

https://dart.dev/tools/sdk/archive#main-channel-url-scheme

@jacobggman
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Projects
None yet
Development

No branches or pull requests

2 participants