refactor: time series collection, resource handling #612
Conversation
| def get_time_series(self, reference_id: str) -> TimeSeries: | ||
| try: | ||
| return TimeSeries( | ||
| reference_id=f"{self.name};{reference_id}", |
There was a problem hiding this comment.
a bit ambigious when both "name+column_name" and column_name is referenced to as reference_id, in different contexts ... can we call it column_name/id here? and that a reference id is "timeseriescollectionname + column name", to get things straight?
There was a problem hiding this comment.
collection_id and time_series_id? We started using reference_id for resources in the api, in that context it's the name of the resource.
I agree it's confusing, the way I see it it's just an id, and an id is expected to have meaning only within the context. I.e. time_series.id and collection.id. But it's always useful to then name that id more specifically in other contexts, where you only have the id (time_series_id and collection_id, etc).
jsolaas
force-pushed
the
refactor/time_series_collection
branch
2 times, most recently
from
24c4650
to
a31867b
Compare
jsolaas
force-pushed
the
refactor/time_series_collection
branch
5 times, most recently
from
ee2d5f6
to
9072503
Compare
| self.header = header | ||
| self.row = row | ||
| super().__init__( | ||
| "Invalid column", |
There was a problem hiding this comment.
possible to indicate which column that is invalid?
There was a problem hiding this comment.
It's handled in the message currently. Not sure what to make of the title in these errors vs message.
| from libecalc.presentation.yaml.yaml_keywords import EcalcYamlKeywords | ||
|
|
||
|
|
||
| class InvalidTimeSeriesResource(InvalidResource): |
There was a problem hiding this comment.
should we use the convention for expceptions to end in Exception or Error? I think we should
Create domain objects for TimeSeriesResource, TimeSeriesCollection++. This should make the behavior more clear, and provide more flexibility in the future. Previously, time_series_collection.py and time_series_collection_mapper.py did a lot of stuff. In addition to dealing with the resource data and validating that, the yaml data was also validated. As this was bundled it was difficult to reuse some of the logic. Each separate step should now be available by using the correct class.
jsolaas
force-pushed
the
refactor/time_series_collection
branch
from
9072503
to
36dc739
Compare
Have you remembered and considered?
docs/docs/changelog/next.md)docs/docs/migration_guides/)BREAKING:in footer or!in header, if breakingECALC-XXXX)Why is this pull request needed?
Previously, time_series_collection.py and
time_series_collection_mapper.py did a lot of stuff. In addition to dealing with the resource data and validating that, the yaml data was also validated. As this was bundled it was difficult to reuse some of the logic. Each separate step should now be available by using the correct class.
What does this pull request change?
Create domain objects for TimeSeriesResource, TimeSeriesCollection++. This should make the behavior more clear, and provide more flexibility in the future.
Issues related to this change: