-
Notifications
You must be signed in to change notification settings - Fork 104
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
Support duplicate plugins #419
Support duplicate plugins #419
Conversation
Codecov Report
@@ Coverage Diff @@
## master #419 +/- ##
==========================================
+ Coverage 93.56% 93.58% +0.01%
==========================================
Files 23 23
Lines 684 686 +2
==========================================
+ Hits 640 642 +2
Misses 44 44
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
i'm inclined towards saying this isn't breaking; i think duplicate plugins was undefined behaviour before (we never stated which would "win"), so i don't think anyone could safely be relying on passing duplicate plugins. |
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.
LGTM
Thanks for picking this up!
great! i bumped the patch version so that once this is merged we can tag a release (since it looks like this packages makes frequent releases on a per-PR basis anyway). also i was expecting to have to do a significant testing lift or hunt down edge cases, but it turns out your initial implementation was ready to go, and really did just need the few lines of tests.... so thank you! |
...not sure why codecov dropped here, but it seems unrelated to this PR. I think this is ready to merge! |
yeah, that's fine -- please could you rebase on master? (another PR went in first, so the version will need bumping one more) |
done! |
Closes #266.
Re-adds the changes started by @nickrobinson251 in #269, with the addition of tests.
Question: Previously, duplicated plugins would be silently removed. Now, they're silently allowed---even in cases where they could potentially lead to undefined behavior (in cases where only one plugin of a certain type is implicitly expected). Not sure what this means for semver: since it's a change in behavior, perhaps it should be a breaking change? But since it was never an explicitly stated behavior, perhaps it's just unbreaking? ...or maybe we should support a flag for falling back on the old behavior? Or throw a warning if duplicate plugins exist? I have no strong opinion here.