Pass in JsonSerializerOptions to generic methods

[MichaelDeutschCoding]

addresses #186

[shacharPash]

@MichaelDeutschCoding do you want to add a test for coverage & show the use case?

[MichaelDeutschCoding]

I added unit tests.

Thanks for fixing that embarrassing typo for me! 🤦‍♂️

[shacharPash]

@MichaelDeutschCoding do you want to fix the errors?
I recommend you to use dotnet test locally

[MichaelDeutschCoding]

Yeah, sorry I was being lazy and just doing it directly in the browser. That does tend towards silly mistakes. I did just clone it locally and run the tests after fixing them. Verified it's working properly now.

Let me know if you'd like more tests and or documentation for this.

[slorello89]

Technically these API changes are binary breaks, if you overlay the new DLLs you'll get missing member exceptions - as the calling applications will need to recompile. NRedisStack is still pre-1.0 so avoiding breaks is still "best effort" I'd say so there's a few options:

1. Add the new signature as an extra overload rather than as an optional parameter
2. Rev the minor version to 0.10.0
3. YOLO - just merge - probably won't break anyone

[shacharPash]

@MichaelDeutschCoding

[MichaelDeutschCoding]

I don't really have the context to know which of those options makes the most sense; this is my first contribution to the code base. I'm happy to implement any of those approaches, if you can just give me the direction you'd like to take.

[shacharPash]

@slorello89
To create another signature where serializerOptions is not optional, we will have to do it like this:

T? Get<T>(RedisKey key, JsonSerializerOptions serializerOptions, string path = "$");

What changes the order of the original function (path after key), I think it's better to leave serializerOptions optional.
WDYT?

[slorello89]

You would just reset this line, and add a new method with:

Task<T?> GetAsync<T>(RedisKey key, string path = "$", JsonSerializerOptions serializerOptions);

as the signature.

[shacharPash]

@slorello89 I didn't understand how this is possible, an optional variable should come at the end

[MichaelDeutschCoding]

I don't know how bad it would be breaking binary/reflection users by changing the method signature, but personally it would feel weird to me to add an additional separate method with a mandatory JsonSerializerOptions, when that parameter is itself optional in the JsonSerializer method we're calling.

If the version number is bumped, does that provide enough "warning" if it does indeed break anyone's use flow?

[shacharPash]

Honestly, I didn't understand why it breaks.
If the variable we added is optional, anyone who used the function without JsonSerializerOptions can continue to use it in exactly the same way because the variable is optional.
@slorello89 what do you say?

[slorello89]

@shacharPash - @MichaelDeutschCoding is correct - adding optional parameters are compiler-compatible but not binary-compatible. You would need to recompile your application if you use the method in question. See my comments above, as this is still not 1.0+ I think all three options are valid (reset the signature, bump minor, or YOLO), however, if we were post 1.0 I'd insist on option 1 as this is a very minor change to make a major revision for and you really shouldn't propagate binary breaks in minors/patches.

[codecov-commenter]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (b178f7a) 94.03% compared to head (26e3bb6) 94.03%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #187   +/-   ##
=======================================
  Coverage   94.03%   94.03%           
=======================================
  Files          82       82           
  Lines        5127     5127           
  Branches      483      483           
=======================================
  Hits         4821     4821           
  Misses        180      180           
  Partials      126      126           

Files	Coverage Δ
src/NRedisStack/Json/JsonCommands.cs	88.27% <100.00%> (ø)
src/NRedisStack/Json/JsonCommandsAsync.cs	88.27% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MichaelDeutschCoding]

Also, once this change may be requiring bumping the version, would we want to consider adding the option to pass in JsonSerializerOptions to the Serialize methods as well?

Certain features like PropertyNamingPolicy, DefaultIgnoreCondition, and Converters (in particular the ability to add a JsonStringEnumConverter) may be very useful to some people.

[shacharPash]

Also, once this change may be requiring bumping the version, would we want to consider adding the option to pass in JsonSerializerOptions to the Serialize methods as well?

Certain features like PropertyNamingPolicy, DefaultIgnoreCondition, and Converters (in particular the ability to add a JsonStringEnumConverter) may be very useful to some people.

@MichaelDeutschCoding Not sure I understood, where else did you think of adding JsonSerializerOptions?

[chayim]

Optional paramter at the end, gets released in a new version of the library. Optional parameters may break ABTs but since they're in new versions, recompilation is fair game. People should pin dependencies.