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

feat: use T -> R type converters for T? -> R? convert #142

Merged
merged 8 commits into from
Oct 31, 2023

Conversation

DevNico
Copy link
Contributor

@DevNico DevNico commented Oct 28, 2023

This will allow usage of T -> R mappers where a mapping of T? -> R? is required. This could technically be a breaking change if someone expected to have null be set instead of the mapped value.

@DevNico DevNico force-pushed the type_converter_nullable branch from 1a11ea0 to d0f63ea Compare October 28, 2023 21:15
@DevNico DevNico changed the title feat: use non-nullable type converters for nullable->nullable convert feat: use T -> R type converters for T? -> R? convert Oct 28, 2023
@tenhobi
Copy link
Member

tenhobi commented Oct 28, 2023

Hello again! I will check this on Monday in more detail. The logic and basically mitigation of how input parameter and return value works in functions is discussed here #128 (comment)

But I will take a look at this case. Could you maybe provide more detail with why etc? 😊

@DevNico
Copy link
Contributor Author

DevNico commented Oct 29, 2023

Sure, after reading #128 (comment) I don't think that it's currently possible to write a single TypeConverter to cover both T -> R and T? -> R? at the same time? The only one that would do that is a TypeConverter<Object?, Object> which I cannot implement for my use case because if my input is null I want my output to be null aswell.

Short example:

class ExampleId extends ValueId<int, ExampleId> {
  const ExampleId(super.value);
}

@AutoMappr(
  [],
  converters: [
    TypeConverter<int, ExampleId>(ExampleId.new),
  ],
)
class ExampleMappr extends $ExampleMappr{
  const ExampleMappr();
}

Sidenote the overview given in #128 (comment) might help some people if it was in the readme.

@tenhobi
Copy link
Member

tenhobi commented Oct 30, 2023

First of all, we can add that list to readme, good point!


I see now, "TypeConverter<Object?, Object>, ... if my input is null I want my output to be null as well".

I extended the support for this also for TypeConverter<Object, Object?>

Aka the proposed change is

  • TypeConverter<Object, Object> ... aka Object converter(Object) => ...
    • Object -> Object
    • Object -> Object?
    • Object? -> Object?, when source is not null <-- THIS
  • TypeConverter<Object, Object?> ... aka Object? converter(Object) => ...
    • Object -> Object?
    • Object? -> Object? when source is not null <-- THIS
  • TypeConverter<Object?, Object> ... aka Object converter(Object?) => ...
    • Object -> Object
    • Object? -> Object
    • Object -> Object?
    • Object? -> Object?
  • TypeConverter<Object?, Object?> ... aka Object? converter(Object?) => ...
    • Object -> Object?
    • Object? -> Object?

@tenhobi
Copy link
Member

tenhobi commented Oct 30, 2023

I am also thinking, should there be an option to disable/enable this, or should we keep it always on?

@tenhobi tenhobi requested a review from petrnymsa October 30, 2023 09:38
tenhobi
tenhobi previously approved these changes Oct 30, 2023
@DevNico
Copy link
Contributor Author

DevNico commented Oct 30, 2023

I am also thinking, should there be an option to disable/enable this, or should we keep it always on?

I do think that the change should always be enabled by default but I am not sure if this is a breaking change. Might be best to release as a new major version. I do not see why one would want to turn this feature off so a feature flag / build configuration value thingy isn't required unless someone asks for it in the future I guess.

tenhobi
tenhobi previously approved these changes Oct 31, 2023
Copy link
Member

@tenhobi tenhobi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the great work @DevNico

@tenhobi tenhobi merged commit 16a5134 into netglade:main Oct 31, 2023
1 check failed
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.

2 participants