Skip to content

Conversation

@bulldetektor
Copy link
Contributor

@bulldetektor bulldetektor commented Mar 21, 2019

Added configuration params for;

  • IImageProcessor.Apply
  • ICloningImageProcessor.CloneAndApply
  • ProcessingExtensions.Mutate
  • ProcessingExtensions.Clone

IImageProcessingContext property MemoryAllocator replaced by Configuration (MemoryAllocator can be retreived from Configuration). Fix #650

@codecov
Copy link

codecov bot commented Mar 21, 2019

Codecov Report

Merging #857 into master will decrease coverage by <.01%.
The diff coverage is 90.16%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
...ImageSharp/Processing/Processors/ImageProcessor.cs 80% <100%> (ø) ⬆️
tests/ImageSharp.Tests/ImageOperationTests.cs 100% <100%> (+2.63%) ⬆️
...arp/Processing/Processors/CloningImageProcessor.cs 70.37% <100%> (ø) ⬆️
tests/ImageSharp.Tests/Processing/DelegateTest.cs 100% <100%> (ø) ⬆️
...ts/Processing/Processors/Transforms/ResizeTests.cs 99.52% <100%> (ø) ⬆️
...ts/ImageSharp.Tests/FakeImageOperationsProvider.cs 100% <100%> (+3.22%) ⬆️
.../ImageSharp.Tests/Processing/ImageProcessorTest.cs 100% <100%> (ø)
...ng/Processing/Processors/Text/DrawTextProcessor.cs 91.66% <100%> (ø) ⬆️
...Sharp/Processing/IImageProcessingContextFactory.cs 100% <100%> (ø) ⬆️
...ssing/Processors/Transforms/AutoOrientProcessor.cs 81.25% <100%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47e8f2c...dd98669. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 21, 2019

Codecov Report

Merging #857 into master will decrease coverage by 0.08%.
The diff coverage is 90.16%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
...ImageSharp/Processing/Processors/ImageProcessor.cs 80% <100%> (ø) ⬆️
tests/ImageSharp.Tests/ImageOperationTests.cs 100% <100%> (+2.63%) ⬆️
...arp/Processing/Processors/CloningImageProcessor.cs 70.37% <100%> (ø) ⬆️
tests/ImageSharp.Tests/Processing/DelegateTest.cs 100% <100%> (ø) ⬆️
...ts/Processing/Processors/Transforms/ResizeTests.cs 99.52% <100%> (ø) ⬆️
...ts/ImageSharp.Tests/FakeImageOperationsProvider.cs 100% <100%> (+3.22%) ⬆️
.../ImageSharp.Tests/Processing/ImageProcessorTest.cs 100% <100%> (ø)
...ng/Processing/Processors/Text/DrawTextProcessor.cs 91.66% <100%> (ø) ⬆️
...Sharp/Processing/IImageProcessingContextFactory.cs 100% <100%> (ø) ⬆️
...ssing/Processors/Transforms/AutoOrientProcessor.cs 81.25% <100%> (ø) ⬆️
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dfa2f9...7b3d206. Read the comment docs.

@antonfirsov
Copy link
Member

Hope I can review this in the next few days, or in the weekend if I can't manage it.
I see no reason to rush with this 😄

@JimBobSquarePants JimBobSquarePants added this to the 1.0.0-rc1 milestone Mar 27, 2019
Copy link
Member

@antonfirsov antonfirsov left a 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 Configuration is passed to a method it should be the first argument (right after a this parameter 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();
Copy link
Member

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.

@JimBobSquarePants
Copy link
Member

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow passing custom Configuration instances to processors

3 participants