add 'dartCompatible' option to MappableClass

[2x2xplz]

This is an attempt to address #82. Unfortunately most other Dart libraries that involve serialization automatically expect toJson()/fromJson() methods to return/accept a Map, rather than a String. I prefer dart_mappable's naming convention, however, as was mentioned in that thread, the Dart convention is widespread.

This adds a dartCompatible option to MappableClass and to the globalOptions that, when true, will rename the methods. Note that if someone uses the renameMethods options, those will take priority. The biggest advantage of this PR is the ability to customize Dart compatibility on each individual class. dartCompatible is false by default so it will not break any compatibility unless you specifically turn it on.

The renaming policy is:

toMap() -> toJson()
toJson() -> serialize()
fromMap() -> fromJson()
fromJson() -> deserialize()

There is also a test group demonstrating a class annotated with dartCompatible: true compared to the default. (I did not write tests for the global option or for Records)

Of course, an alternate philosophy is to decide that going forward, this package should aim for widespread Dart compatibility. One could rename all the generated methods to comply, and require a legacy: true option in order to preserve the current naming convention. This would break compatibility, but easily fixed via

global_options:
  dart_mappable_builder:
    options:
      legacy: true

A PR that followed this concept would be basically the same PR but with every boolean flipped.

[schultek]

Hi. First of all thanks for this PR and the feedback that goes with it.

As I wrote in my last comment on the issue, my decision to keep the current naming (although with an escape hatch to revert back to the legacy naming) is final as it stands right now. I also do not intend to change the way the escape hatch works for these reasons (which I reserve the right to set as the maintainer):

1. Using this feature is explicitly discouraged and should therefore also be explicitly not "as easy as possible" (like setting a boolean). I accept the fact that sometimes its needed and thats what the escape hatch is for. It shouldn't be a first class feature of the package though.
2. I don't see a good enough reason to expand the escape hatch to a class by class basis. Also I don't think it would be beneficial to have since then you end up with inconsistent interfaces between mappable classes, which will result in more problems.

[2x2xplz]

Absolutely, I understand and respect your viewpoint on this issue. Like I said, I prefer this package's naming convention. Just to address your 2nd bullet point, the motivation, for me, was to utilize dart_mappable classes with the retrofit package.

retrofit requires toJson() to return a map. It even posts a reminder during codegen:
https://github.com/trevorwang/retrofit.dart/blob/bf38e05a1b070eb09eb0ce8c0791d71a414dbf8e/generator/lib/src/generator.dart#L1671
The class-by-class renaming would allow a developer to use dart_mappable like normal, except for the classes which will be serialized/deserialized with retrofit. However, as you note, this produces inconsistent interfaces.

Thanks for your response, and for your work on dart_mappable. Happy holidays!

[rydmike]

@2x2xplz and @schultek is there any solution to getting dart_mappable to play nice with Retrofit and Chopper.

I have tried doing the reconfig:

global_options:
  dart_mappable_builder:
    options:
      renameMethods:
        fromJson: deserialize
        toJson: serialize
        fromMap: fromJson
        toMap: toJson

But I'm still getting the retrofit requires toJson() to return a map error.

EDIT:

Experimented a bit and got it working.
Made a test repo with the config I used. It also compares the setup to Freezed + Json_serializable and just json_serializable.

If anybody lands here with this issue, here is the repo: https://github.com/rydmike/mapper_issue

[schultek]

I see, it was a builder ordering problem.

This is a great example and comparison of packages. This could also be of value to issue #82, maybe you can also comment this there so people reading the issue see it.