-
-
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Order and filter processors by supported commands #183
Order and filter processors by supported commands #183
Conversation
I just tested this PR again (with browser caching off) and noticed it always returned -1 as lowest index... Now it uses an intersection of parsed commands and commands supported by the processor, so it really gets the first index of a supported command 馃槃 Because case sensitivity of the commands depend on the comparer used in the dictionary returned by It's probably also fine to always use |
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.
Thanks for the PR. I need to do a thorough review + perf check this but I've made some notes.
Codecov Report
@@ Coverage Diff @@
## master #183 +/- ##
==========================================
- Coverage 84.71% 84.65% -0.06%
==========================================
Files 50 50
Lines 1426 1440 +14
Branches 190 194 +4
==========================================
+ Hits 1208 1219 +11
- Misses 164 166 +2
- Partials 54 55 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 think you're onto something here but I think we can reduce the workload.
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.
Thanks for your help here @ronaldbarendse I'll get an official release out asap with these changes.
Prerequisites
Description
This fixes issue #182 by ordering and filtering the processors by supported commands.
The biggest change this introduces is not calling the
Process(...)
method on processors where theCommands
collection doesn't contain a parsed command.