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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨πŸ”₯ Implement new converter API #191

Merged
merged 14 commits into from Nov 18, 2023
Merged

Conversation

pleonex
Copy link
Member

@pleonex pleonex commented Aug 18, 2023

Description

This PR proposes a new API to use converters on IFormat variables and via the Node class. It tries to address the issues collected from users where the current API can be very verbose or confusing.

The new API is:

static object ConvertFormat.With(Type converterType, dynamic src, params object[] args);

Node Node.TransformWith<TConv>();
// Node Node.TransformWith<TConv>(params object[] args); won't be added, not type safe
Node Node.TransformWith(IConverter converter);
// Node Node.TransformWith<TSrc, TDst>(IConverter<TSrc, TDst> converter); // won't be added, generics can't be omitted
Node Node.TransformWith(Type converterType, params object[] args);

// Extension methods in IFormat
// generics can be omitted, they are inferred from the converter variable and source variable (if it's not object)
TDst ConvertWith<TSrc, TDst>(this TSrc, IConverter<TSrc, TDst> converter, bool disposeInput = false, bool disposeConverter = false);

Examples at the bottom

Main changes:

  • Extension method for IFormat allowing fluent-like chain conversions: myFormat.ConvertWith(new Converter1()).ConvertWith(new Converter2())
  • The new API returns the destination type. No need of casting.
  • The new API requires passing the converter object, instead of the type in a generic: po.ConvertWith(new Po2Binary(args))
  • Remove IInitializer<T>. Use instead the constructor to pass parameters or initialize the converter.
    • There wasn't a clean solution to have type checking without having to specify the type.
    • It could cause issues if thread-safe was required on a converter
    • It's more natural in C# to use constructor to pass parameters. It could work with DI if needed some day.
  • ConvertFormat.To() and Node.TransformTo() are removed.
    • It was confusing and non-deterministic when they were working or throwing exceptions (and hard to debug why).
    • It's safer to always provide the converter type if we know in advance.
    • Generic tools any way has to deal with the use case of having more than one converter for a type.
    • They are removed instead of marked as obsolete, so we can remove the dependency with MEF in the core library.
  • Small changes:
    • Bump dependency of code analyzers and fix some warnings
    • Improve exception message for invalid converters.

This new design does not require having dependencies in the core library (Yarhl) with an assembly scanning tech or DI (like MEF)

Cons

  • We may get warnings if the converter implements IDisposable saying we didn't dispose the object. I added the argument in the extension method but the analyzer won't detect it.
    • As a workaround we could create the converters beforehand with using var and pass the objects
  • I know it's ugly (and even not good practice) to create a new variable in-line in the method arguments (e.g.: ConvertWith(new Po2Binary(arg1)) vs ConvertWith<Po2Binary>(arg1) which is cooler) but we couldn't find a way to make it work and being type-safe.
  • If the converter implements more than once IConverter<,>, then the generics in the extension method ConvertWith cannot be omitted.

Options considered

  • A fluent-like API would be even more verbose and redundant po.ConvertWith<Po2Binary, Args>(args).To<BinaryFormat>() vs po.ConvertWith(new Po2Binary(args))
  • Re-designing the API as a dependency-injection
    • It doesn't play well with converters that accepts different arguments of simple types (string, int), constructors couldn't be used for initialization.
    • In DI designs, the instances are created by requests of other constructors, their constructor arguments don't change. In our case we create them transient-like from our methods, not constructors and different arguments each time. We would need to workaround creating scopes, injecting arguments and then requesting.
    • No benefit.

Requirements analyzed

Description Old API New API
Core library (Yarhl) does not require assembly scanning ❌ βœ…
API does not depend on assemblies in the exe folder (ConvertTo) ❌ βœ…
Generic software (non type aware, like UI) can convert formats and nodes βœ… βœ…
Conversion of IFormat can be chained for multiple conversions ❌ βœ…
The format from a Node can be converted without affecting the node easily ❌ βœ…
Conversion does not require casting to work with the destination type ❌ βœ…
Converters can accept arguments βœ… βœ…
Converters can be made thread-safe ❌ βœ…
Type safety for input/source object ❌ βœ…
Type safety for converter (implements IConverter<,>) ❌ βœ…
Type safety for destination type with converter ❌ βœ…
Type safety for arguments βœ… βœ…
Compile error if missing arguments in converter ❌ βœ…

TODO

  • Architecture design
  • Draft implementation
  • Create PR and push NuGet
  • Create tests
  • Update release notes
  • Consider Node.TransformWith<TConv>(args)
  • Fix / Replace MEF (plugin discovery) Future PR. PoC in SceneGate
  • Performance tests
  • Wait for feedback

Example

// Vanilla API for formats with destination type returned (no need of ConvertFormat)
BinaryFormat binary = new Po2Binary().Convert(po);

// IFormat extension methods
BinaryFormat binary = po.ConvertWith(new Po2Binary());

StringFormatTest test2 = stringFormat
        .ConvertWith(new String2Int())
        .ConvertWith(new Int2String("thisIsAParam"));

// Node
poNode.TransformWith<Po, BinaryFormat>(new Po2Binary()); // source type is not known, so we can't omit generics
poNode.TransformWith(new Po2Binary()); // it uses the `IConverter` overload which is slower (use dynamic)
poNode.TransformWith<Po2Binary>(); // only when a converter has a parameterless constructor

// Arguments on a converter
var options = new Po2BinaryOptions();  // this class doesn't exist (yet?)
BinaryFormat binary = po.ConvertWith(new Po2Binary(options));
poNode.TransformWith<Po, BinaryFormat>(new Po2Binary(options));

// Converting the format of a Node without changing its format
Image<Rgba32> fontImage = overlay11.Format
    .ConvertWith(new Font2Binary(FontKind.Debug))
    .ConvertWith(new Font2Image(), disposeInput: true);

// APIs when we don't know the types at compile-time (generic tools)
Type converterType = ...; // selected via UI or file
object[] args = ...; // deserialized from file or UI

node.TransformWith(converterType);
node.TransformWith(converterType, args);

object result = ConvertFormat.With(converterType, input, args);

Performance results

TransformWith<TSrc, TDst>(IConverter<TSrc, TDst> converter) vs TransformWith(IConverter converter) (uses dynamic):

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22621
12th Gen Intel Core i7-1260P, 1 CPU, 16 logical and 12 physical cores
.NET SDK=7.0.400
  [Host]     : .NET 6.0.21 (6.0.2123.36311), X64 RyuJIT
  DefaultJob : .NET 6.0.21 (6.0.2123.36311), X64 RyuJIT
Method Mean Error StdDev Gen 0 Allocated
TransformWithDynamic 204.9 ns 1.74 ns 1.54 ns 0.0229 216 B
TransformWithTyped 189.8 ns 2.53 ns 2.37 ns 0.0229 216 B

For now most of the old API is not removed but marked them as obsolete. It allows to do a smooth migration part by part.

Please feel free to provide feedback and try in your projects.

I have temporary modify the build pipeline so there is a NuGet version for this branch available in the preview feed.
Version: 4.0.0-PullRequest0191.191 (the last digit may increase)

I'll leave this PR open some time while we test it in our projects, so we can validate it.

Internal validation:

This closes #124

@pleonex pleonex added this to the vNext milestone Aug 18, 2023
@pleonex pleonex self-assigned this Aug 18, 2023
@pleonex pleonex marked this pull request as draft August 18, 2023 17:17
@pleonex
Copy link
Member Author

pleonex commented Aug 19, 2023

Good feedback from trying in some projects (PokΓ©mon Conquest and Ekona):

  • API is cleaner.
  • No compile errors, only obsolete warnings
  • All tests still passed except when using the TransformTo, ConvertFormat.To because some converters have now constructors with parameters (not very often).
  • Forces me to follow good practices of having only one IConverter<,> per class.
  • It allows a smooth migration:
    1. Replace TransformTo and ConvertFormat.To APIs to the new API.
    2. First update converters to replace IInitializer<T> with a constructor: move same code and allow to use readonly, reference null checks and remove initialization checks in Convert()
    3. Then, convert usages of the converters with the new API
  • However migration takes some time: ~1-2 hours per big project.
  • Some times VS AI picks up the type of changes and it's just a tab-pressing to update a line.
  • I like the constructurs because it's a bit harder to miss potential parameters rather than knowing there is an "Initialize" method.

@pleonex pleonex marked this pull request as ready for review November 18, 2023 10:09
@pleonex pleonex removed the wip label Nov 18, 2023
@pleonex pleonex merged commit c616cc0 into develop Nov 18, 2023
5 checks passed
@pleonex pleonex deleted the feature/new-converter-api branch November 18, 2023 10:34
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.

Review converters fluent API
1 participant