Closed ajnelson-nist closed 4 months ago
(This PR is a follow-on from review of #45 .)
@ajnelson-nist everything is clear and elegant, the only doubt concerns the following code:
- call_participant: Union[None, ObservableObject, List[ObservableObject]] = None,
...
+ call_participant: Union[None, dict, List[dict]] = None,
...
The dict ObservableObject has the method get_id() while the generic dict may not have it so at runtime it would raise an error. It might be fine anyway, the caller will be in charge to use the right parameter.
@ajnelson-nist everything is clear and elegant, the only doubt concerns the following code:
- call_participant: Union[None, ObservableObject, List[ObservableObject]] = None, ... + call_participant: Union[None, dict, List[dict]] = None, ...
The dict ObservableObject has the method get_id() while the generic dict may not have it so at runtime it would raise an error. It might be fine anyway, the caller will be in charge to use the right parameter.
@fabrizio-turchi , thank you for the feedback.
Your remark about get_id()
, leading to recalling other methods special to the case_mappings
class hierarchy that specializes dict
, further confirms to me that this code base should encourage using its specialized classes rather than permitting plain dict
s.
If you agree, please leave an approving Review (no description text needed).
This Pull Request is posted for discussion. I'm fine with it being merged, but please do not merge with less than two GitHub approvals.
The first patch in this PR enacts the following change:
Here is a breakdown of the effects:
This denotes
call_from
is an optional singleton-valued parameter.This denotes
call_participant
andcall_to
are optional parameters, which can be a singletonObservableObject
or a list ofObservableObject
s.One side-effect of this patch benefitting type safety of the generated graph objects is that the type system enforces that
ObservableObject
s are used in these fields. That is, it would be an error flagged by the type system to pass auco.identity.Person
object here. (While the ontology currently would permit an object to be multi-typed asPerson
andObservableObject
, there is a coming proposal to disallow this and several other conflations. For example, UCO should never accidentally type a node asPerson
andFile
; but, how to implement this in SHACL and/or OWL is taking some thought.)A side-effect of this patch is that type-checking will fail on any downstream code that tries passing a raw JSON object. I.e. this code currently in
example.py
is fine ...... and, this would also be fine (classes inlined because I realized they aren't in this code base yet) ...
... but, someone bypassing the objects hierarchy in this code base would get an error from static type review on trying this:
The error from the current type-checking in this code base is:
I'm uncertain how likely this last use case is to occur. But, if it's something this code base should support, then this patch could keep the type system happy ...
... but I don't think it's possible to then make use of the Python class hierarchy for type safety. E.g. that
Person
graph objct could go in to the Python code, and would have to be caught by SHACL validation to realize it's not anObservableObject
.In summary: If the type system is to be used to check cardinalities, a decision will have to also be made on type enforcement in methods' parameters.
If the goal of this code base is to catch errors in bindings before run time, the type system is an option. If instead the goal is to not generate incorrect graphs,
assert
s in object-initialization, list-appends, or property setter methods could meet the cardinality enforcement goal and bypass Python typing questions.