-
-
Notifications
You must be signed in to change notification settings - Fork 889
#650 Allow passing of custom Configuration instances to processors #857
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
#650 Allow passing of custom Configuration instances to processors #857
Conversation
…ssor<T>.Apply() and changed IImageProcessingContext<TPixel> prop MemoryAllocator to Configuration
Codecov Report
@@ Coverage Diff @@
## master #857 +/- ##
==========================================
- Coverage 88.89% 88.89% -0.01%
==========================================
Files 1014 1015 +1
Lines 44295 44321 +26
Branches 3208 3215 +7
==========================================
+ Hits 39376 39399 +23
+ Misses 4198 4196 -2
- Partials 721 726 +5
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #857 +/- ##
==========================================
- Coverage 88.98% 88.89% -0.09%
==========================================
Files 1017 1015 -2
Lines 44464 44321 -143
Branches 3228 3215 -13
==========================================
- Hits 39566 39399 -167
- Misses 4176 4196 +20
- Partials 722 726 +4
Continue to review full report at Codecov.
|
|
Hope I can review this in the next few days, or in the weekend if I can't manage it. |
antonfirsov
left a comment
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.
- Unfortunately it's not yet explicitly defined, but our guideline suggests avoiding default arguments
- Additionally: when a
Configurationis passed to a method it should be the first argument (right after athisparameter in an extension method).
Consequence:
We should avoid passing configuration = null, if not provided, let's dispatch the instance that is most relevant in the given context in the calling method where no custom Configuration instance is passed.
| using (ImageFrame<TPixel> temp = source.Frames.RootFrame.Clone()) | ||
| { | ||
| Configuration configuration = source.GetConfiguration(); | ||
| configuration = configuration ?? source.GetConfiguration(); |
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.
Obtaining the Configuration instance should not be the responsibility of individual processors. This is error prone!
The Configuration instance we consume in this method should not be null, but the instance we expect in this context.
|
@bulldetektor Thanks for your work in this PR. I'm going to close it now in favour of #1070 . It was far too much to ask of you to update after all the churn in this repo! |
Added configuration params for;
IImageProcessingContext property MemoryAllocator replaced by Configuration (MemoryAllocator can be retreived from Configuration). Fix #650