Minor error message improvement: record positional fields

[marcglasberg]

Currently, I'm getting this error message:

celest\functions\portfolio.dart:38:30: 
The return type of a function must be serializable as JSON. Positional fields are not supported in record types

38 │ Future<(Stock, CashBalance)> sellStock(
   │                              ^^^^^^^^^

Very minor, but maybe the error messages could follow the format in this order: What. Why (when applicable). How to fix it.

Currently you have Why. What and no How.

In What/Why/How format it becomes:

Your function returns a record type with positional fields, which is not serializable as JSON.
Use only named fields in the record type.

---

A question: Do you feel that anything that doesn't have very "natural" JSON representation should not be allowed? Because technically it is serializable with $1 and $2 just like Dart does:

{ 
   "$1": <first value>, 
   "$2" : <second value>, 
}

Or even:

{"values": [<first value>, <second value>]}

Do you feel this may create problems?

[dnys1]

Hey @marcglasberg, great feedback on the error messages. I like the model you're describing of what/why/how to fix 👍

Positional record fields can be serialized as you describe, and this is how package:json_serializable handles them. We chose not to support them for the reason that we want Celest APIs to be accessible and debuggable with any API client and the $1, $2 semantics made the API boundary difficult to reason about outside of the hermetic Celest client.

We also felt that using named parameters required a minimal amount of additional code (Stock, CashBalance) -> ({Stock stock, CashBalance balance}) (which can be DRY'd with typedefs) while encouraging better API design. In your example, I can only guess at the meaning of the Stock and CashBalance return values.

Please let me know if we are being too opinionated here, though, and if the use of positional record fields would improve your experience.

[marcglasberg]

I think your argument about being accessible and debuggable makes perfect sense, and I'm happy with it. But it's not always that we have control over the types we use. Obligatory named params means you won't allow third-party types that use positional parameters. Which may force me to convert to/from the internal third-party classes and my own classes that I can send over to Celest.

Or maybe actually it won't force me to do that! I just need to override the toJson/fromJson with your new extended types override feature, right?

[dnys1]

Yes! I think the solution to this is overrides which can define a custom fromJson/toJson which only applies to your backend.

So given the following type:

// Defined in another package
class BadType {
  const BadType(this.positionalFields);

  final (String a, String b) positionalFields;
}

You could have an override like the following:

@override
extension type FixBadType(BadType it) implements BadType {
  factory FixBadType.fromJson(Map<String, Object?> json) {
    final positionalFields = json['positionalFields'] as Map<String, Object?>;
    return BadType(
      (positionalFields['a'] as String, positionalFields['b'] as String),
    ) as FixBadType;
  }

  Map<String, Object?> toJson() {
    return {
      'positionalFields': {
        'a': positionalFields.$1,
        'b': positionalFields.$2,
      },
    };
  }
}

I will close this for now, but feel free to reopen in the future if custom overrides don't fully solve the problem!

[marcglasberg]

@dnys1 maybe please still leave it open? We went on a tangent, but the issue was just about a minor improvement in the error message, by applying the What/Why/How format.

[dnys1]

Whoops! Yes, we can