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

[Workspace] Rename command resolves with FileStat #12278

Conversation

FernandoAscencio
Copy link
Contributor

@FernandoAscencio FernandoAscencio commented Mar 9, 2023

What it does

Closes #12101
Rename command now resolves with FileStatWithMetadata on success and undefined when not.

How to test

The second commit includes changes for testing.

  1. Start Theia
  2. Right click the file to rename
  3. Select Test: File Rename in the context menu
  4. Check results are as expected.

image

Review checklist

Reminder for reviewers

@FernandoAscencio FernandoAscencio force-pushed the fa/12101ResolveWithFileStat branch 2 times, most recently from 2f78e82 to 923cac4 Compare March 10, 2023 14:58
@vince-fugnitto
Copy link
Member

cc @kittaakos

@vince-fugnitto vince-fugnitto added commands issues related to application commands workspace issues related to the workspace labels Mar 13, 2023
@kittaakos
Copy link
Contributor

Thanks for the fix. I checked out 923cac4 and verified it with the electron example app.

How to test

The second commit includes changes for testing.

  1. Start Theia
  2. Right click the file to rename
  3. Select Test: File Rename in the context menu
  4. Check results are as expected.

image

Interesting way to verify the changeset 🙃 I see the URI in the notification with 923cac4. I cannot reproduce the undefined counter-example, but this is expected.

12278.mp4

@FernandoAscencio
Copy link
Contributor Author

@kittaakos
The undefined example should happen when you cancel the name change.

@kittaakos
Copy link
Contributor

The undefined example should happen when you cancel the name change.

Verified. Thank you!

12278.mp4

@FernandoAscencio FernandoAscencio force-pushed the fa/12101ResolveWithFileStat branch from 923cac4 to 370be95 Compare March 29, 2023 14:26
@FernandoAscencio
Copy link
Contributor Author

The testing commit has been dropped for merging.

This commit closes eclipse-theia#12101.

Signed-Off-By: FernandoAscencio <fernando.ascencio.cama@ericsson.com>
@FernandoAscencio FernandoAscencio force-pushed the fa/12101ResolveWithFileStat branch from 370be95 to fda1dea Compare March 29, 2023 14:28
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍

  • confirmed that the rename command works as expected for files and folders
  • confirmed that the stat is returned on successful renames

@vince-fugnitto vince-fugnitto merged commit 964a022 into eclipse-theia:master Mar 29, 2023
@vince-fugnitto vince-fugnitto added this to the 1.36.0 milestone Mar 29, 2023
@vince-fugnitto vince-fugnitto deleted the fa/12101ResolveWithFileStat branch March 29, 2023 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commands issues related to application commands workspace issues related to the workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[workspace]: file.rename command should resolve with the file stat after the successful execution
3 participants