netglade / auto_mappr

Code generation for mapping between different objects with ease.
https://pub.dev/packages/auto_mappr
MIT License
71 stars 7 forks source link

Generated code for classes without ctors ignores 'from' argument #190

Closed qwertylolman closed 7 months ago

qwertylolman commented 8 months ago

Describe the bug For classes without ctors, when Field has 'from' argument - the generated builder function is incorrect. If I'm not using the 'from' argument everything is generated as expected. Also, seems like 'custom' argument behaves the same way.

To Reproduce

// mapper config
  MapType<FuzzDTO, FooDTO>(
    fields: [
      Field('label1', from: 'anotherLabel1'),
      Field('label2', from: 'anotherLabel2'),
      Field('items', custom: DataMapper.mapCustomItems),
    ],
  ),

@freezed
class FuzzDTO with _$FuzzDTO {
  const factory FuzzDTO({
    required String anotherLabel1,
    required String anotherLabel2,
    required List<BuzzDTO> anotherItems,
  }) = _FuzzDTO;

  factory FuzzDTO.fromJson(Map<String, dynamic> json) => _$FuzzDTOFromJson(json);
}

// part of isar, that's why it has Collection annotation
// this code generates the expected output and works fine
@Collection(accessor: 'foos')
class FooDTO {
  Id id = Isar.autoIncrement;

  final String label1;
  final String label2;
  final List<BarDTO> items;

  FilterDTO({
    required this.label1,
    required this.label2,
    required this.items,
  });
}

// but this code generates the wrong builder function
@Collection(accessor: 'foos')
class FooDTO {
  Id id = Isar.autoIncrement;

  late String label1;
  late String label2;
  late List<BarDTO> items;
}

// builder function's return value looks like this:
return _i6.FooDTO();

Expected behavior Generated builder function is correct and has all required mappings

Version info auto_mappr 2.3.0 auto_mappr_annotation 2.1.0

tenhobi commented 7 months ago

Hi @qwertylolman, thanks for the issue. Just to make sure, you are mapping from CategoryDTO to FilterDTO, but you didn't provide an example of CategoryDTO, could you? Also there is something weird going in FooDTO since it has FilterDTO constructor. So if you may fix the code 🙏

qwertylolman commented 7 months ago

Hi @qwertylolman, thanks for the issue. Just to make sure, you are mapping from CategoryDTO to FilterDTO, but you didn't provide an example of CategoryDTO, could you? Also there is something weird going in FooDTO since it has FilterDTO constructor. So if you may fix the code 🙏

Hi. Sorry, I was simplifying the DTOs for example, and forgot to change names in mapping. I have updated the source code example.

qwertylolman commented 7 months ago

btw, as a feature request. it would be nice to have extra warnings (maybe errors?) during codegen for classes without ctors. something to notify that not all fields are initialized. in the scenario above I found out that something is wrong only during the runtime, as flutter throws an error that field is not initialized.

tenhobi commented 7 months ago

For the case without constructor, this seems like something that should work.

It is strange because we can assign fields using their setters. https://github.com/netglade/auto_mappr/blob/main/packages/auto_mappr/test/integration/fixture/mapping_to_target.dart

But for sure we might check whether that works even for final fields. 🤔

tenhobi commented 7 months ago

btw, as a feature request. it would be nice to have extra warnings (maybe errors?) during codegen for classes without ctors. something to notify that not all fields are initialized. in the scenario above I found out that something is wrong only during the runtime, as flutter throws an error that field is not initialized.

I am not sure whether it should be warning, or only info, but yea, we probably might add logs to such cases like missing constructor, check whether all fields have been initialized (aka check late fields) etc. It might not be a bad thing, since you might set those late fields elsewhere, but I see the issue that it might just help to see a log about that!

qwertylolman commented 7 months ago

For the case without constructor, this seems like something that should work.

It is strange because we can assign fields using their setters. https://github.com/netglade/auto_mappr/blob/main/packages/auto_mappr/test/integration/fixture/mapping_to_target.dart

But for sure we might check whether that works even for final fields. 🤔

issue appears with classes without ctors and with 'late' fields. and only when you are configuring the 'from'. generated code for the same class but configured for mapping without 'from' is working great and as expected.

tenhobi commented 7 months ago

Hi @qwertylolman, sorry for the delay, I have been out of town. I fixed the issue and now renaming (from) also work when setting using setters. See #192. Could you validate that on your code? 🙏

Closing for now, but feel free to reopen if you find any issue.