-
Notifications
You must be signed in to change notification settings - Fork 293
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
Multiroot improvements #443
Conversation
Pull Request Test Coverage Report for Build 533
💛 - Coveralls |
@connectdotz please take a look. How do you find this changes: should i add |
probably update changelog since many users have been asking for these new commands |
Yes, but i’m considering remove that commit and let @vdh to update his PR. So i guess i’ll add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't run it but the change seems pretty straight forward... once you get the build pass I will run it and approve.
src/extensionManager.ts
Outdated
const { commonPluginSettings: { disabledWorkspaceFolders } } = this | ||
if (this.extByWorkspace.has(workspaceFolderName)) { | ||
return false | ||
} else if (disabledWorkspaceFolders.includes(workspaceFolderName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for else
, you can also consider consolidating these 2 conditions into a single if statement, might be more readable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this with the same motivation :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reason for else
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, missed that. No reason for else
, i'll split it in two separate conditions.
@escaton I've rebased the PR, although since this is my first contribution it'd be great to get someone double check the logic of the Promise-based changes I've made. |
Dropped my commits related to start/stop/restart commands |
…eFolders change on the fly
Is there any overview of the features here? |
there is a brief introduction in the README.md, if it's not intuitive then maybe we should consider adding more... |
@connectdotz would you like to merge this PR to |
yes, was waiting for the restart-command PR, now it is in, is there anything you want to change in this PR before I merge it? |
done :) |
* use Map to store jest instances * remove enbledWorkspaceFolders, update README, handle disabledWorkspaceFolders change on the fly * review fixes * Fix typo
add start-all, stop-all, restart{-all} commandsdisabledWorkspaceFolders
change on the fly#trivial