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

Added case insensitive fields mapping #18

Merged
merged 35 commits into from Aug 18, 2021
Merged

Conversation

Antriadus
Copy link
Contributor

No description provided.

@Antriadus
Copy link
Contributor Author

Antriadus commented Jun 24, 2021

Hi, @smotastic what's your approach to unit testing mapper generator? It works locally, with paths instead of version in pubspec.yaml.
What can we do to make those tests pass?

@smotastic
Copy link
Owner

Hi @Antriadus , first of all I like the feature. Let's see if we can make the tests work again.

I think it has to do with the updated version.
You are referencing 1.1.4 in the generator, but this version hasn't been published yet, so the CI doesn't know what to download, because for it it does not exist yet.

I would suggest going back to 1.1.3, and referencing the local smartstruct annotations in your generator pubspec.

smartstruct: 
  path: ../smartstruct

Then the tests in the smartstruct_generator will resolve the local version of the smartstruct package.
I'll also add a comment on the line in the code.

I will then, for my release update the version accordingly in both packages. And as soon as I published it on pub.dev, the generator should be fine for having 1.1.4 in its dependencies too.
But this will all be done by my internal releasescript.

@Antriadus
Copy link
Contributor Author

Please check now

@smotastic
Copy link
Owner

Hi, the tests look good now.
I'll be a little busy in the next weeks, but I'll try to check this out as soon as I can.

Copy link
Owner

@smotastic smotastic left a comment

Choose a reason for hiding this comment

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

Edit: Added as proper review

generator/pubspec.yaml Outdated Show resolved Hide resolved
Copy link
Owner

@smotastic smotastic left a comment

Choose a reason for hiding this comment

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

Edit: Added as proper review

Copy link
Owner

@smotastic smotastic left a comment

Choose a reason for hiding this comment

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

Addes as proper review

Copy link
Owner

@smotastic smotastic left a comment

Choose a reason for hiding this comment

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

Is it possible to split these two features (case insensitive mapping and custom mapping) into two branches? As there are a lot of changes it's difficult to have an overview of what was done


class Target {
final String fullName;
final DateTime birthDate;
Copy link
Owner

Choose a reason for hiding this comment

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

Only constructor parameters are paid attention to by the generator.
If i change the Target class in the example custom_mapping.dart to the following.

class Target {
  final String fullName;
  DateTime? birthDate;
  final String avatar;

  Target(
    this.fullName,
    this.avatar,
  );
}

With the custom mapping of birthdate (not changed)

@Mapper()
abstract class CustomMapper {
  @DateTimeFromSourceMapper(targetField: "birthDate", f: mapBirthDate)
  @CustomMapping(targetField: "fullName", f: mapFullName)
  Target fromSource(Source source);
}

The birthDate is ignored by the generator.

///
/// Annotate the method with [CustomMapping] and provide a valid targetField and mapping function f
/// To achive type safety, override CustomMapping class with specific TargetField and SourceClass types
class CustomMapping<TargetField, SourceClass> {
Copy link
Owner

Choose a reason for hiding this comment

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

Is it possible to extend the Mapping Annotation, instead of writing a new CustomMapping Annotation?
Basically it's an enhanced Mapping(...), because instead of mapping the target to another source property, we are mapping the target to another method.
The generator could just check if the source argument is available as a property in the Source Parameter, and if not check if there is a method and use this instead

///
/// Annotate the method with [CustomMapping] and provide a valid targetField and mapping function f
/// To achive type safety, override CustomMapping class with specific TargetField and SourceClass types
class CustomMapping<TargetField, SourceClass> {
Copy link
Owner

Choose a reason for hiding this comment

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

Right now it's not possible to support multiple source parameters in the custom mapping annotation.
This is currently not supported by the mapper anyway, but planned in the future (#7)
so it would be nice if the customMapping would already support this (but not using it right now).

This also means, if we will have a dynamic amount of source parameters, we cannot define a static amount of parameters in the CustomMapping,
but dynamic is also not possible via generics.
So my first idea would be to not pass the custom mapping method as a parameter itself, but only the name of the method.
Something like

@Mapper()
abstract class CustomMapper {
  @CustomMapping(targetField: "fullName", f: 'mapFullName')
  Target fromSource(Source source);
}

The Mapper then could easily locate the method by it's name and just pass all source parameters which are available.
Alternatively we could define multiple CustomMapping Annotations like CustomMapping1, CustomMapping2, ..., which would then accept a multiple amount of source parameters,
but this sounds a little dirty and probably more complex to handle when we are reading them.

@smotastic smotastic changed the base branch from master to develop August 18, 2021 06:12
@smotastic smotastic merged commit d76a2e7 into smotastic:develop Aug 18, 2021
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.

None yet

3 participants