Obsolete SetMemoryAllocator builder extensions #154
Conversation
Codecov Report
@@ Coverage Diff @@
## master #154 +/- ##
==========================================
+ Coverage 84.57% 84.82% +0.24%
==========================================
Files 46 46
Lines 1368 1364 -4
Branches 177 177
==========================================
Hits 1157 1157
+ Misses 163 159 -4
Partials 48 48
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -65,7 +65,7 @@ public static IImageSharpBuilder SetMemoryAllocator(this IImageSharpBuilder buil | |||
| /// <param name="builder">The core builder.</param> | |||
| /// <returns>The <see cref="IImageSharpBuilder"/>.</returns> | |||
| [Obsolete("Use ImageSharp.Configuration.MemoryAllocator. This will be removed in a future version.")] | |||
There was a problem hiding this comment.
Should we make this an error instead? Otherwise I think we should keep the implementation to not break the usage?
There was a problem hiding this comment.
as far as I can tell the current implementation would just register something in the DI, that is never retrieved, i.e. used.
So the only breaking, that I could think of with it, is if a user was using it as a convenience method to register a TMemoryAllocator and also then retrieving it from DI for their own purposes.
So here I just make it a noop instead.
But if you can think of another scenario?
There was a problem hiding this comment.
I did not check the implementation details. Just noticed that the code was removed but would not break the build. I am still wondering if we should add Error = true to make sure the user changes their code.
There was a problem hiding this comment.
Ah I get the suggestion now. Up to your preference really.
The code that is removed didn't do anything (hasn't done for a long time I suspect), so it is questionably / technically an error to call it.
I tend more towards not breaking builds / forcing users to change code where possible, so didn't go with error.
But as you prefer?
There was a problem hiding this comment.
Will leave this up to @JimBobSquarePants to decide.
There was a problem hiding this comment.
I have less sense than you both so would have just deleted it. :)
I doubt anyone is using this since it doesn't do anything other than provide a convenient way to register an allocator for their own usage. Lets make it an error.
There was a problem hiding this comment.
Now I just want to delete it too ;)
Changed to Error
|
It鈥檒l be a few days til I can look at this. Sick at the moment with oral shingles. |
Prerequisites
Description
Obsoletes
SetMemoryAllocator(from #152) - that's caused me confusion in the past before too :)I was going to remove the comments, but then stylecop doesn't like it.
Could update them to mention the obsolete message if preferable?
Could also just remove them, instead of obsolete...