[monitorlib] Clean up geotemporal business objects #316
Conversation
| def __init__(self, iterator: Iterator[dict]): | ||
| parsed_values = [ | ||
| v if isinstance(v, Volume4D) else ImplicitDict.parse(v, Volume4D) | ||
| for v in iterator | ||
| ] | ||
| super(Volume4DCollection, self).__init__(parsed_values) |
There was a problem hiding this comment.
Thanks, but actually this constructor code will go away with this fix :)
|
|
||
| @property | ||
| def rect_bounds(self) -> s2sphere.LatLngRect: | ||
| if not self.volumes: | ||
| if not self: |
There was a problem hiding this comment.
nit: Is this meant to catch the case where the collection is an empty list? If so I find it more explicit to use len(self) == 0, as I don't believe there should another case where self evaluates as False
There was a problem hiding this comment.
Checking emptiness with if not x is Pythonic per PEP8 ("For sequences, (strings, lists, tuples), use the fact that empty sequences are false"), but I agree in many cases, and in one or two instances, I've been bitten by the fact that checking this way does not always work properly in combination with other checks. So, I use both methods depending on the circumstance.
Clean up geotemporal business objects 079c92b
This PR works on cleaning up our geotemporal business objects. Doing this will hopefully make things easier and clearer going forward. The main changes are:
Regarding the TCollections, ImplicitDict's jsonschema generation doesn't currently support types that inherit non-dict types whose schema can be defined (e.g.,
class Volume4DCollection(List[Volume4D]):). This is fixed in this PR, but in the mean time, the JSON Schema will still work, they just won't describe the content in those classes. I suggest that it's worth moving forward with this cleanup before fixing the jsonschema generation, and fixing jsonschema generation can be a follow-on task. When that fix is incorporated, the constructors of the two TCollection objects in this PR can be removed.