@JsonCodable() supports super toJson and fromJson. #89
Conversation
davidmorgan
force-pushed
the
json-superclasses
branch
from
dbc871e to
b584bbe
Compare
davidmorgan
force-pushed
the
json-superclasses
branch
from
b584bbe to
9fbd365
Compare
| /// | ||
| /// TODO(davidmorgan): is this the right place to provide access to supertypes? | ||
| QualifiedName supertypeOf(QualifiedName name) { | ||
| return _constructSuperTypes(_lookupNamed(name.asString), []).first.name; |
There was a problem hiding this comment.
For types, I think this is the right place to put it. A QualifiedName alone is not really a type though, interface types would also need the instantiation. And this method answers class hierarchy queries, which are not exactly the same as type hierarchy queries:
- There is no designated "primary" supertype in the type hierarchy since it's not relevant when checking for subtype relationships. Using
.firsthere essentially relies on an implementation detail. - This generates an intermediate fully-resolved parent type when we only care about the name (so at the very least has to run in the none scope I think?)
I absolutely think that they're should be methods to resolve super types of interface types in TypeSystem. I'm not sure if the QualifiedName is close enough to an interface type to also belong here though.
Especially if we care about the type of subclass relationship, maybe it's cleaner to add extends, implements and mixesIn properties on Interface as well?
In my mind, the kinds of question a type system should answer are "is this class, through whatever how many intermediates, a subtype of JsonConverter?". IMHO, "does this class extend from a class defining a fromJson constructor" is a different kind of question that is more related to the AST instead of a type model. But that might just be my opinion, so feel free to disagree or ignore this if it doesn't make sense :)
There was a problem hiding this comment.
All good points, thanks! I agree re: "what does this extend" being more like an AST question than a type model, I think moving this functionality to Interface is the right direction.
There's already a lot going on in this PR so added a TODO pointing to this comment + an item to the short term planning issue.
schemas/dart_model.schema.json
Outdated
| }, | ||
| "namedParameters": { | ||
| "type": "array", | ||
| "description": "The named positional parameters of this member, if it has them.", |
There was a problem hiding this comment.
| "description": "The named positional parameters of this member, if it has them.", | |
| "description": "The named parameters of this member, if it has them.", |
schemas/dart_model.schema.json
Outdated
| "$ref": "#/$defs/NamedFunctionTypeParameter" | ||
| } | ||
| }, | ||
| "parameters": { |
There was a problem hiding this comment.
I am confused by this one, what does it represent?
There was a problem hiding this comment.
That seems to be a mistake, removed :)
| extension ExecutableElementExtension on ExecutableElement { | ||
| List<StaticTypeDesc> requiredPositionalParameters( | ||
| AnalyzerTypesToMacros translator, TypeTranslationContext context) { | ||
| return parameters |
There was a problem hiding this comment.
Fwiw, I would use list literals for this. But, I don't know that we have any specific guideline.
Add checking of the super type for
fromJsonandtoJson, which requires adding query support for constructors and methods.I just added them flat in
Member, there is still the TODO to add more types there, this gives something useful for new types to do, i.e. differentiate constructors, methods and fields :)This also needs to look up the supertype, @simolus3 I wasn't quite sure where that should go, added to
StaticTypeSystemhere, or did you have something else in mind? :)