-
Notifications
You must be signed in to change notification settings - Fork 101
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
Enable all non-experimental modules on plugin activation #191
Enable all non-experimental modules on plugin activation #191
Conversation
@eclarke1 Can you please add "Needs Review" and other relevant labels as well? |
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.
@kirtangajjar This looks like a solid start. I left a few comments and suggestions for parts that need iteration.
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.
@kirtangajjar I've added those few requested changes and with that this looks good to me. Thank you for the PR! 🎉
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.
Overall looks great, few minor comments.
Bail if option is set with existing value Co-authored-by: Crisoforo Gaspar Hernández <hello@crisoforo.com>
Thank you @felixarntz @mitogh for your reviews. I have committed the suggestion made by @mitogh. Please do let me know incase this needs something else from my end :) |
@kirtangajjar Sorry, see my comment in #191 (comment), I'm not sure that change actually makes sense. From what I can tell it was better the way you originally had it:
|
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.
@kirtangajjar One thing remaining here per the above.
cc @mitogh
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@felixarntz Fixed. Sorry didn't think that one through fully. |
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.
Thank you @kirtangajjar for the PR - awesome work! 🙌
Summary
Fixes #61. Enable all non-experimental modules on plugin activation.
Fix is quiet simple so there isn't much to explain here.