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 Type Converters #65

Closed
wants to merge 12 commits into from

Conversation

DevNico
Copy link
Contributor

@DevNico DevNico commented May 2, 2023

Close #60

This is just a rough first implementation of the feature with very few tests etc. etc.

I'm open for any input about how things should work.

Another addition I want to propose is specifying field names (or even patterns) for Type Converters. This would allow globally specifying a UserId TypeConverter that converts from String to UserId if the field has the name userId.

Comment on lines 73 to 97
@AutoMappr(
[
MapType<DateDto, DateDomain>(
fields: [
Field(
'dateTimeA',
type: TypeConverter<String, DateTime>(Mappr.tryParseFirstOfDecemberInYear),
),
],
types: [
TypeConverter<String, DateTime>(Mappr.parseSecondOfDecemberInYear),
],
),
MapType<DateDto2, DateDomain>(),
MapType<UserDto, User>(
fields: [
Field('id', type: TypeConverter<String, UserId>(UserId.new)),
Field('accountId', type: TypeConverter<String, AccountId>(AccountId.new)),
],
),
],
types: [
TypeConverter<String, DateTime>(Mappr.parseISO8601),
],
)
Copy link
Member

Choose a reason for hiding this comment

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

Is there any benefit of doing TypeConverter<String, AccountId>(SomeFunction.xxx) instead of just passing SomeFunction.xxx?

Copy link
Contributor Author

@DevNico DevNico May 2, 2023

Choose a reason for hiding this comment

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

Nope probably not the generic could probably be moved to the function level. I'll try that

Copy link
Contributor Author

@DevNico DevNico May 2, 2023

Choose a reason for hiding this comment

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

Actually you need the explicit definition because inference doesn't seem to work without it. The wrapper Object is also there to extend it with further configuration options.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure right now, but isn't it similar to custom where it also works for functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it would probably work without the wrapper object. But then we'd lose the option to configure the TypeConverter further with e.g. field name constraints.

For brevity we could support both passing just the function or the whole Object I guess. What do you think?

@DevNico
Copy link
Contributor Author

DevNico commented May 2, 2023

About constraining TypeConverters to specific fields. I'd like to support both matching the field User.id somehow and userId at the same time. A possible solution I see is allowing User.id as a field name constraint in the TypeConverter. Field names in Dart cannot have a . in them so there probably wouldn't be a conflict.

The . notation is something the smartstruct package generally supports. Maybe this would be a good addition in general?

Also I see two options for constraining a single TypeConverter to multiple field names. Either we support Regex patterns or we allow specifying a list of allowed field names.

@tenhobi
Copy link
Member

tenhobi commented May 2, 2023

You mean #1, right? Aka flattening

@tenhobi
Copy link
Member

tenhobi commented May 2, 2023

If we can have type convertors for fields, maptype, automapper, do we need further filtering by field?

@DevNico
Copy link
Contributor Author

DevNico commented May 2, 2023

Yes I do think that would still a good or even required addition. Imagine having many different XyzId classes which all have a String backing type. I think it would be much cleaner to specify one converter on the mappr level thats restricted to both e.g. User.id and userId instead of having to do that multiple times. Actually nevermind about that part since the type filtering already solves this.

Additionally lets say across all models you have a createdAt and updatedAt timestamp in ISO 8601 format but also many timestamps in a different format. I feel like filtering by field would greatly reduce the boilerplate to implement these use cases.

@tenhobi
Copy link
Member

tenhobi commented May 2, 2023

What do you think, @petrnymsa?

@DevNico DevNico force-pushed the feature/type-converters branch from dabc193 to dbc285d Compare May 2, 2023 17:49
@petrnymsa
Copy link
Member

Yes I do think that would still a good or even required addition. Imagine having many different XyzId classes which all have a String backing type. I think it would be much cleaner to specify one converter on the mappr level thats restricted to both e.g. User.id and userId instead of having to do that multiple times. Actually nevermind about that part since the type filtering already solves this.

Additionally lets say across all models you have a createdAt and updatedAt timestamp in ISO 8601 format but also many timestamps in a different format. I feel like filtering by field would greatly reduce the boilerplate to implement these use cases.

I am kind of against to have every possible feature in the package, but overall I like idea of TypeConverters.

Taking your example with timestamps - I dont think that this is something, that you should solve with AutoMappr, as it is more "workaround of bad app design" than solution. If some arbirtrary API returns different Date formats, you should transfer it into proper format and in the domain level of your application you should have, one defined format for all date times etc. ...

So, I am definitely for this feature if it is just

  • Defining TypeConverter between two types, where this converter is used when there is mapping between two non-matching types
  • TypeConverter can be defined on
    • AutoMappr level - globally used typeconverter<A,B> , where whenever there is mapping from A to B, converter is used
    • MapType level - converter used only for concrete MapType instance

On the Field level - I dont think it is needed - you can acomplish any transformation with "custom"

But there is one more issue to discuss

Suppose we have mapping from A -> B, and A has nested class C, B has nested D, .e.g

class A {  
  final C foo;
}

class B {
 final D foo;
}

class C {} 
class D {}

Right now, AutoMappr will try to automatically map from C->D if there is defined MapType<C,D>.

With TypeConverters we can potentially have another way to solve nested mapping

  • Define MapType<C,D> - mappr will take responsibility of mapping instance of C to D
  • Define Field('foo'...) with custom mapping for C -> D
  • Define TypeConverter<C,D> where we have responsibility to define how C should be mapped to D

This can be maybe little bit confusing, so it should be clarified that

  • If user wants use mappr for mapping C to D on its own (and not only for nested mapping) - use MapType
  • If user does not care about mapping C -> D directly, but it is needed for nested mapping - use TypeConverter

Moreover, if is it second case, we already have issue #6, ideally we should not need to defined these nested mappings if theirs purpose is only, and only, for nested mapping...

@tenhobi
Copy link
Member

tenhobi commented May 3, 2023

That's a good point that we could probably allow doing MapType<String, DateTime>, extend mapping so if fields are matched by name and types are not the same to take a look if there is some mapping it can use, and handle "type conversion" in its custom function.

So the question is, how are MapType and TypeConverter semantically different? Especially when we want to be able to make it work with non-primitives.

@DevNico
Copy link
Contributor Author

DevNico commented May 3, 2023

Taking your example with timestamps - I dont think that this is something, that you should solve with AutoMappr, as it is more "workaround of bad app design" than solution. If some arbirtrary API returns different Date formats, you should transfer it into proper format and in the domain level of your application you should have, one defined format for all date times etc. ...

I do think this is something my mapping code should take care of. Most of the time you don't have the luxury of a great API with sensible formats for values. In our case we mostly generate Dart API Clients from openapi specs and then provide an intermediary that maps these generated APIs to domain level defined ones. Mapping these generated DTOs to our domain objects is exactly the use case I'd expect a mapping library to cover. That includes having the option to map badly designed types.

On the Field level - I dont think it is needed - you can acomplish any transformation with "custom"

I agree, there was no use case as of yet where that was necessary.

So the question is, how are MapType and TypeConverter semantically different? Especially when we want to be able to make it work with non-primitives.

I'm still in the process of replacing my existing mapping code base with auto_mappr and that exact question also appeared and I've actually "abused" the TypeConverter in one case to act just like a MapType. I agree that it's probably better to somewhat extend the existing API instead of providing another construct. This is something I've noticed by just implementing this first design idea. I'll try to write some examples of the challenges I encountered when replacing custom mapping code with codegen as soon as I find the time. It'll probably take a week or two tho since I'll be quite busy for now.

@wstrange
Copy link

Any updates on type converters? This is a real pain point right now.

@tenhobi
Copy link
Member

tenhobi commented Aug 20, 2023

Hi @wstrange, please lets move the discussion to #60 or maybe on Discord. 🙏 However, while in summer there are vacations etc., we didn't have time to decide nor implement this feature yet.

@tenhobi
Copy link
Member

tenhobi commented Sep 8, 2023

Will be implemented in a different PR, since this PR depends on way too old code. Note that there is a proposal for this feature in last comments of #60.

@tenhobi tenhobi closed this Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow specifying type converters
4 participants