hiranthaR / Json-to-Dart-Model

Json to Dart Model extension can convert JSON objects into Dart data classes. It supports pure Dart class conversion, Flutter-recommended JSON serialization using annotations, Freezed support, Effective Dart:Style, and many more features. Currently, it has more than 135,000 installs.
https://marketplace.visualstudio.com/items?itemName=hirantha.json-to-dart
MIT License
93 stars 18 forks source link

[BUG] double conversion #58

Closed Miamoto-Musashi closed 2 years ago

Miamoto-Musashi commented 3 years ago

Is there an existing issue for this?

Current Behavior

during json conversion of doubles the code does

double a = json['a'] as double

in case the double does not contains a .0 the dart assumes is an int and gives error; a better conversion I think would be

double a = json['a']==null ? null: json['a'].toDouble()

Expected Behavior

parse with no error

Steps To Reproduce

having a jsonc file with this snippet

{
      "length": 1450.8,
      "width": 1200.8,
      "volume": 255.8
    }

create the models

tran try to parse a json like:

{
      "length": 14,
      "width": 120,
      "volume": 255
    }

Version

3.3.6

Relevant JSON syntax

{
      "length": 1450.8,
      "width": 1200.8,
      "volume": 255.8
    }

Anything else?

No response

iamarnas commented 3 years ago

@Miamoto-Musashi Hi 👋

Thanks for this report. It was just these days that I was thinking about how to improve it, and I come to the this solution.

Example:

// Parse to integer.
(json['someKey'] as num).toInt();
// Parse to double.
(json['someKey'] as num).toDouble() ;
// And I think it should work with null check. 
(json['someKey'] as num)?.toDouble() ; 

The parsers should correct the numbers. What do you think?

And with integers maybe it's not needs. Only with doubles for better performance.

iamarnas commented 3 years ago

@Miamoto-Musashi can you test my methods?

iamarnas commented 3 years ago

@Miamoto-Musashi Hi. In my tests, it works as expected without a problem. What about a generator that if the JSON value returns integer then the generator generates it as an integer. It is not possible to predict that the user wants to convert the integer to double. You must tell to the generator by casting .0 if you want a double value. Otherwise, you as a developer must convert values by initializing.

Example:

 int a = 10; // <- result from the JSON
 double b = a as double; // or a.toDouble(); <- initialize.

Looks like you did not use dart null safety.

double a = json['a'] as double;
double a = json['a'] == null ? null : json['a'].toDouble(); // <- this

double a = json['a'] as double? // <- this in the Dart with null safety is equal to the example above.
Miamoto-Musashi commented 3 years ago

Hi @iamarnas, in my tests for some reason the short json['key']??json['key'].toDouble() version does not work; only the long json['key']==null?null:json['key'].toDouble() version works, strange behavior...

You can predict what kind of value the developer want by inspecting the original field, in my case I'm trying to get back a field declared double in my class

"wallet": {
      "amount":99.99 <-- converted to double
  }
iamarnas commented 3 years ago

@Miamoto-Musashi But this 99.99 is double value and of course it will be converted as double. When you adding null check then your not have error. I feel you do something wrong by overriding fromJson() method. And it throw the error with all doubles for you?

I can implement null check for all values. But with Dart null-safety it not needs. Dart return null if value marked as nullable by ?.

Miamoto-Musashi commented 3 years ago

the converter detect is double but before the conversion was as double now the toDouble() works better

iamarnas commented 3 years ago

@Miamoto-Musashi I do still not believe that casting as double? does not work for you.

Miamoto-Musashi commented 3 years ago

Hi @iamarnas I don't enable Null-safety to introduce json serializable I would need null safety or older version that would mess with my dependencies, can't try now. I can confirm null aware operator fails while simple version does not

does not fails

amount: json['amount'] == null ? null : json['amount'].toDouble(),

fails

amount: json['amount'] ?? json['amount']?.toDouble(),

I assume dart is making some shortcut on the null-aware version that does not work properly...

this is the model definition

{
        "key": "5621f8a9-f05a-4469-bddf-0cdb56b24771-1",
        "requestKey": "5621f8a9-f05a-4469-bddf-0cdb56b24771-3",
        "amount": 2300.99, //amount in local currency (field currency)
        "currency": "EUR", // currency
        "userKey": "user",
        "d@isAccepted": false,
        "d@isCreated": false,
        "d@isWon": false
}
iamarnas commented 3 years ago

@Miamoto-Musashi Hi 👋 Ok. You still use Dart without Null-safety. Then you can be forced null check sometimes. I not thought to implement it for older Dart SDK because with null-safety it not needs. But I can add some option to enable it for all properties.

iamarnas commented 3 years ago

@Miamoto-Musashi Hi. Can you test this method if it work.

(json['someKey'] as num)?.toDouble();
iamarnas commented 3 years ago

@Miamoto-Musashi Hi 👋 I have implemented double values conversion following Google principles. But to add if null check feels like it is not worth it because Dart language with null safety does not need it and most of the users I think migrating to Null-Safety and this method will be unuseful in the future.

Miamoto-Musashi commented 3 years ago

@iamarnas I understand your thoughts, pls keep in mind that one of the reason I started using your extension is exactly the availability of json conversions without null safety; the reason is because going trough the null safety upgrade if you come from an old version is a nightmare and in this scenario I was looking for something to avoid json_serializable since it produce null-safety version of the code and force you to have 2.12 flutter version.

iamarnas commented 3 years ago

@iamarnas I understand your thoughts, pls keep in mind that one of the reason I started using your extension is exactly the availability of json conversions without null safety; the reason is because going trough the null safety upgrade if you come from an old version is a nightmare and in this scenario I was looking for something to avoid json_serializable since it produce null-safety version of the code and force you to have 2.12 flutter version.

@Miamoto-Musashi Ok, I will add an option for null check for non-null safety Dart < 2.12. It is not so much work. Maybe it will be done today.

Miamoto-Musashi commented 3 years ago

@iamarnas probably you can drive the choice checking the user setting to produce null-safe or not-null-safe code

iamarnas commented 3 years ago

@Miamoto-Musashi I will add an option in the settingsincludeIfNull = false. You will need just to enable it and it will work only if null safety is disabled.

iamarnas commented 3 years ago

@Miamoto-Musashi and only for fromJson method. Or you need for toJson to ?

Miamoto-Musashi commented 3 years ago

@iamarnas I think you don't need a new option, you can use the jsonToDart.nullSafety setting

iamarnas commented 3 years ago

@iamarnas I think you don't need a new option, you can use the jsonToDart.nullSafety setting

@Miamoto-Musashi I know, but maybe someone wants to keep a cleaner code.

iamarnas commented 3 years ago

@Miamoto-Musashi what about toJson method?

Miamoto-Musashi commented 3 years ago

yes I mean if the user checked the setting to go for null-safe version you produce the code with null-safe otherwise you produce the null check code, is more important to have a version that does not brake before it is clean. in toJson is the same the setting will drive one version or the other depending by null-safe

iamarnas commented 3 years ago

@Miamoto-Musashi Ok I think you are right.

iamarnas commented 3 years ago

@Miamoto-Musashi Maybe better I will add an option, because this generator if null safety is enabled then generates everything for dart null safety it cant be enabled for both versions att the same time.

iamarnas commented 3 years ago

@Miamoto-Musashi It was published, please tell me if some can be improved better. For example, include it in the toJson method.

iamarnas commented 2 years ago

Closing it because the generator supports it. Welcome to reopen it with more suggestions.