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

Consistent delete behavior from command palette #826

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

moxw
Copy link
Contributor

@moxw moxw commented Sep 21, 2023

Fixes: #799

feat(deleteAppMaps.ts): add closeEditorByUri function after deleting appmaps
feat(contextMenu.ts): update deleteAppMap command to handle absence of item object
feat(deleteAppMap.test.ts): add new integration test for deleteAppMap
fix(closeEditorByUri.test.ts): fix test to include input conditions

@ahtrotta ahtrotta self-requested a review September 22, 2023 14:12
Copy link
Contributor

@ahtrotta ahtrotta left a comment

Choose a reason for hiding this comment

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

This isn't working consistently for me (I'm on Linux, specifically Debian). To reproduce:

  1. Make a fresh clone of spring-petclinic in a fresh directory with a unique path that has never been used (VS Code will cache certain information about your project if you use the same project path).
  2. Make sure you have the Extension Pack for Java installed in VS Code.
  3. When it asks you to use Maven or Gradle, select Maven:
    image
  4. Open the Test Runner and run the system tests:
    image
  5. You should see these AppMaps:
    image
  6. Run Delete All AppMaps from the Command Pallette:
    image
  7. The AppMaps do not disappear from the AppMaps tree view even though they are removed from the filesystem:
    image

@moxw moxw force-pushed the fix/consistent-delete-behavior branch from 08d569d to 0466e11 Compare September 26, 2023 21:06
…appmaps

feat(contextMenu.ts): update deleteAppMap command to handle absence of item object
feat(deleteAppMap.test.ts): add new integration test for deleteAppMap
fix(closeEditorByUri.test.ts): fix test to include input conditions
feat(deleteAppMaps.ts): add clear method to AppMapCollection after closing editors
feat(appmapCollection.ts): introduce clear method in AppMapCollection interface
feat(appmapCollectionFile.ts): implement clear method for AppMapCollectionFile class
@moxw moxw force-pushed the fix/consistent-delete-behavior branch from 0466e11 to 98649f9 Compare September 26, 2023 21:11
@moxw
Copy link
Contributor Author

moxw commented Sep 26, 2023

This isn't working consistently for me (I'm on Linux, specifically Debian). To reproduce:

  1. Make a fresh clone of spring-petclinic in a fresh directory with a unique path that has never been used (VS Code will cache certain information about your project if you use the same project path).
  2. Make sure you have the Extension Pack for Java installed in VS Code.
  3. When it asks you to use Maven or Gradle, select Maven:
    image
  4. Open the Test Runner and run the system tests:
    image
  5. You should see these AppMaps:
    image
  6. Run Delete All AppMaps from the Command Pallette:
    image
  7. The AppMaps do not disappear from the AppMaps tree view even though they are removed from the filesystem:
    image

I wasn't able to reproduce this exactly step-by-step (the tests didn't complete explore appmaps) but I could see this happening in some other scenarios. So, I have added a clear method to clear the tree when deleteAllAppMaps is called. This should take care of all the scenarios.

@moxw moxw requested a review from ahtrotta September 26, 2023 21:15
Copy link
Contributor

@ahtrotta ahtrotta left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@ahtrotta ahtrotta merged commit d77083d into develop Sep 27, 2023
2 checks passed
@ahtrotta ahtrotta deleted the fix/consistent-delete-behavior branch September 27, 2023 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deletion of AppMaps in VS Code using the context menu doesn’t work consistently
2 participants