Skip to content

Conversation

alenakhineika
Copy link
Contributor

@alenakhineika alenakhineika commented Aug 25, 2020

Description

  • Add playgrounds explorer to the tree views
  • Search for .mongodb files in the current workspace including all nested folders and excluding folders and files specified in MongoDB extension settings
  • User can manage the list of excluded folders and files
  • Support the refresh action in the playgrounds explorer contextual menu
  • Support the create a new playground action in the playgrounds explorer contextual menu
  • Open the playground by clicking on it or using a contextual menu
  • For connections use the connections explorer header instead of the connections tree root

Kapture 2020-08-25 at 17 24 28 4

Screenshot 2020-08-25 at 17 11 56

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@alenakhineika alenakhineika requested a review from Anemy August 25, 2020 17:16
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

This is really good. nice. I see this getting a lot of good use. Nice work adding the setting for folder/file exclusions too - with wildcards! Left two small questions/ideas.

Also if it isn't too much work, maybe we could add one more test to the playgroundsExplorer.test.ts to make sure we're using the excludeFromPlaygroundsSearch setting correctly (more just to future proof us against small changes that might change behavior unexpectedly).

Something I need to test at some point - or have you done this by chance - is to make sure when the settings for the views are disabled like mdb.showMongoDBConnectionExplorer are set to false (the views are hidden) that vscode doesn't call getChildren on the views.

package.json Outdated
"title": "Refresh"
},
{
"command": "mdb.createPlaygroundFromTreeView",
Copy link
Member

Choose a reason for hiding this comment

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

Do we use this command currently? Looks like it might be an extra.

Copy link
Member

Choose a reason for hiding this comment

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

Just this comment - then lgtm!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, i thought i deleted it! Now deleted for sure :)

@Anemy
Copy link
Member

Anemy commented Aug 27, 2020

Hmm what should happen if a file was deleted, and the playgrounds list hasn't been refreshed yet, and the now non-existant playground is clicked on?
I'm thinking maybe we can show a prompt or something, currently it silently fails and passes an error to console:
Screen Shot 2020-08-27 at 10 28 34 AM

@alenakhineika
Copy link
Contributor Author

@Anemy I tested mdb.showMongoDBConnectionExplorer and showMongoDBPlaygrounds manually and they don't call getChildren if set to false. I didn't find a good way hot to cover it with unit tests, so we can add them later. Fell free to add them later as well, if you will have some ideas.

Updated PR to address other comments, thx for the valuable feedback!

Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

One small command tweak that's in the command palette, otherwise lgtm!
Nice work on this.

"title": "MongoDB: Create MongoDB Playground"
},
{
"command": "mdb.refreshPlaygrounds",
Copy link
Member

Choose a reason for hiding this comment

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

Ah I think this one makes it into the command palette.

...
"editor/title": [{
  "command": "mdb. refreshPlaygrounds",
  "when": "false"
}
...]

Is it something we want in the command palette? In that case maybe we could add a command for that which has the MongoDB: prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added additional command for refreshing playgrounds from the command palette in addition to one is being called from the tree view. Plus test for it.

Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

lgtm!

@alenakhineika alenakhineika merged commit 22e0dac into master Sep 1, 2020
@alenakhineika alenakhineika deleted the VSCODE-145/playgrounds-and-connections-updated branch September 1, 2020 13:33
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.

2 participants