221 ngsi ld migrate v2 datamodels in ld #234
Conversation
There was a problem hiding this comment.
Very cool addition!
Some general questions for my understanding:
- Why is it necessary to have distinct named and unnamed versions? E.g.
NamedContextPropertyandContextProperty. I would assume that a named version is preferable and that a name would be created by default, if not given by the user. - This is not limited to the LD branch in filip, but is the prefix "Context" necessary? E.g. couldn't we just say
NamedPropertyorProperty, since a context is anyway implied when we use fiware?
filip/models/ngsi_ld/base.py
Outdated
| Returns: | ||
|
|
||
| """ | ||
| return q |
filip/models/ngsi_ld/context.py
Outdated
| if value == "Relationship": | ||
| value == "Relationship" | ||
| elif value == "TemporalProperty": | ||
| value == "TemporalProperty" |
There was a problem hiding this comment.
Why no warning and reassignment of value here?
filip/models/ngsi_ld/context.py
Outdated
| elif value == "TemporalProperty": | ||
| value == "TemporalProperty" | ||
| else: | ||
| logging.warning(msg='NGSI_LD Properties must have type "Property"') |
There was a problem hiding this comment.
Also mention in the message that the type will be forcibly set as "Property"?
filip/models/ngsi_ld/context.py
Outdated
| elif value == "TemporalProperty": | ||
| value == "TemporalProperty" | ||
| else: | ||
| logging.warning(msg='NGSI_LD GeoProperties must have type "GeoProperty" ' |
There was a problem hiding this comment.
What scenario would cause this check to be executed even if no GeoProperties were used?
filip/models/ngsi_ld/context.py
Outdated
| In the NGSI-LD data model, properties have a name, the type "Geoproperty" and a value. | ||
| """ | ||
| name: str = Field( | ||
| titel="Property name", |
There was a problem hiding this comment.
| titel="Property name", | |
| titel="GeoProperty name", |
filip/models/ngsi_ld/context.py
Outdated
| return value | ||
|
|
||
|
|
||
| class ContextGeoProperty(BaseModel): |
There was a problem hiding this comment.
How come ContextGeoProperty doesn't inherit from ContextProperty?
filip/models/ngsi_ld/context.py
Outdated
| def add_attributes(self, **kwargs): | ||
| """ | ||
| Invalid in NGSI-LD | ||
| """ | ||
| raise NotImplementedError( | ||
| "This method should not be used in NGSI-LD") |
There was a problem hiding this comment.
Nice idea to add this for users coming from ngsi-v2!
Maybe also add a hint, so something like "This method should not be used in NGSI-LD. Try using add_properties instead."
tests/models/test_ngsi_ld_context.py
Outdated
| def test_entity_delete_attributes(self): | ||
| """ | ||
| Test the delete_attributes methode |
There was a problem hiding this comment.
Rename to ..._delete_properties?
| example3_temporalQ = { | ||
| "timerel": "between", | ||
| "timeAt": "2017-12-13T14:20:00Z" | ||
| } | ||
| with self.assertRaises(ValueError): | ||
| TemporalQuery.model_validate(example3_temporalQ) |
djs0109
changed the base branch from
NGSI-LD
to
212-testcase-for-every-endpoint-1
|
The data models have been mostly implemented. The fine-tuning will be done together with the endpoints. To this end, this PR is closed. |
close #221 #220