Skip to content
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

Make converters injectable and respect culture #117

Merged
merged 13 commits into from
Sep 4, 2020

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Sep 3, 2020

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 馃懏.
  • I have provided test coverage for my change (where applicable)

Description

Apologies in advance, this touches a lot of files. Coverage is good though.

This is one of those breaking changes we unfortunately have to implement before V1

The current CommandParser was not injectable and also did not respect the current culture. Respecting culture is a required feature for using in many CMS implementations.

By default the system will use InvariantCulture so there are no surprises for people upgrading but there is a switch in the middleware options UseInvariantParsingCulture which allows using the CultureInfo.CurrentCulture.

Converters and Processors both required method signature changes to allow the update.

I also found and fixed two issues

  1. We were disposing of the output stream outside of the write lock
  2. We were not returning after a head request.
  • Refactor converters, parser, and processors to allow current culture
  • Update tests
  • Add additional tests

@codecov
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #117 into master will increase coverage by 2.24%.
The diff coverage is 92.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   84.64%   86.89%   +2.24%     
==========================================
  Files          46       44       -2     
  Lines        1146     1190      +44     
  Branches      159      164       +5     
==========================================
+ Hits          970     1034      +64     
+ Misses        128      106      -22     
- Partials       48       50       +2     
Flag Coverage 螖
#unittests 86.89% <92.94%> (+2.24%) 猬嗭笍

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage 螖
src/ImageSharp.Web/Commands/TypeConstants.cs 100.00% <酶> (酶)
...rc/ImageSharp.Web/Processors/FormatWebProcessor.cs 100.00% <酶> (酶)
...b/Commands/Converters/SimpleCommandConverter{T}.cs 75.00% <66.66%> (+3.57%) 猬嗭笍
.../ImageSharp.Web/Middleware/ImageSharpMiddleware.cs 79.26% <69.56%> (+0.32%) 猬嗭笍
...ageSharp.Web/Commands/Converters/ColorConverter.cs 86.66% <84.61%> (-1.80%) 猬囷笍
...harp.Web/Middleware/ImageSharpMiddlewareOptions.cs 78.00% <87.50%> (+14.36%) 猬嗭笍
src/ImageSharp.Web/Commands/CommandParser.cs 95.65% <95.23%> (-2.63%) 猬囷笍
...Sharp.Web/Commands/Converters/ArrayConverter{T}.cs 100.00% <100.00%> (酶)
...mageSharp.Web/Commands/Converters/EnumConverter.cs 85.71% <100.00%> (+1.09%) 猬嗭笍
.../Commands/Converters/IntegralNumberConverter{T}.cs 100.00% <100.00%> (酶)
... and 9 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 b4b3fd5...d0b2e10. Read the comment docs.

@JimBobSquarePants JimBobSquarePants added this to the 1.0.0 milestone Sep 3, 2020
@JimBobSquarePants JimBobSquarePants changed the title WIP Make converters injectable and respect culture Make converters injectable and respect culture Sep 3, 2020
@JimBobSquarePants JimBobSquarePants requested a review from a team September 3, 2020 23:26
@JimBobSquarePants JimBobSquarePants marked this pull request as ready for review September 4, 2020 00:03
Copy link
Member

@tocsoft tocsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the one comment on Enum handling but all the rest is looking good to me.

Only other thing that I have a small issue (mostly an aesthetics thing and not something I would explicitly want changing unless you wanted to) is the DefaultServicesAreRegistered tests... that to me is screaming out to be a theory test taking in the Service and implementation types as arguments.

src/ImageSharp.Web/Commands/CommandParser.cs Outdated Show resolved Hide resolved
@JimBobSquarePants
Copy link
Member Author

@tocsoft I updated the parser to allow custom enum types also fixed some boxing allocations caused by the parser/converter API

Agree re the test but left it ugly as-is for now as I didn't want to end up potentially fighting Xunit skipping theory data values because it thinks they're all the same. Time is precious! 馃槃

@JimBobSquarePants JimBobSquarePants merged commit 72c2cb6 into master Sep 4, 2020
@JimBobSquarePants JimBobSquarePants deleted the js/injectable-culture-converters branch September 4, 2020 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants