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: enable to json for every serializable model #213

Merged
merged 2 commits into from
Apr 27, 2024

Conversation

KRTirtho
Copy link
Contributor

Closes #212

@hayribakici
Copy link
Collaborator

@KRTirtho that's a useful feature! Could you check the CI build for any errors?

@rinukkusu
Copy link
Owner

I think I/we have removed it to lower the warnings and better our dart analyzer score regarding unused code. Of course we can always revisit this topic. Currently I have no hard feelings regarding toJson or not toJson.

@hayribakici
Copy link
Collaborator

hayribakici commented Apr 14, 2024

@rinukkusu is it possible to somehow "configure" whether a toJson should exist or not? Maybe through pubspec?

@KRTirtho
Copy link
Contributor Author

Sorry for the late reply. I'll fix the CI issues asap. (Forgot to add @override for some toJson lol)

I think I/we have removed it to lower the warnings and better our dart analyzer score regarding unused code.

As far as I know, if unused methods are not private, it shouldn't decrease pub performance score. Because libraries are supposed to offer methods to the user which can unused by the library itself.

@hayribakici hayribakici requested a review from rinukkusu April 19, 2024 20:26
@KRTirtho
Copy link
Contributor Author

Btw, in case you forgot to check, I fixed the CI issues 👍

Copy link
Owner

@rinukkusu rinukkusu left a comment

Choose a reason for hiding this comment

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

Looks good from what I can tell! Thanks for contributing!

@rinukkusu rinukkusu merged commit 188b5fa into rinukkusu:master Apr 27, 2024
2 checks passed
@rinukkusu
Copy link
Owner

@KRTirtho released with v0.13.6

@KRTirtho
Copy link
Contributor Author

That's great news 🎉

But, we have a issue with toJson of child property not being called by default. jsonEncode calls toJson on Object that are not primitve before calling toString, so we don't face any problem. But when we encode json without dart's default, we face the <Type Name> is not Map<String, dynamic> exception.

As you can see, .toJson is not called for externalUrls property for _$$AlbumToJson

'external_urls': instance.externalUrls,

Adding following in build.yaml fixes the issue

targets:
  $default:
    builders:
      json_serializable:
        options:
          any_map: true
          explicit_to_json: true

I'll create a PR with the simple fix. Sorry for the trouble, I didn't know this issue until I used Hive to cache some responses.
Tbh, explicit_to_json should've been true by default. Not sure why it's turned off in the first place

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.

Enable createToJson for the models
3 participants