Add departure and arrival datetime to Direction #116
Conversation
khamaileon
force-pushed
the
departure-arrival-time
branch
3 times, most recently
from
0d73dd1
to
4886c83
Compare
khamaileon
force-pushed
the
departure-arrival-time
branch
from
4886c83
to
227c5d3
Compare
|
For the moment, it works with OTP and Google. I haven't found the corresponding fields in Valhalla. I'd like to improve the unit tests before adding more providers. |
|
Do we really need timezone support for the client? The way we do it in Valhalla is to only accept local time (after all, any snapped graph location‘s timezone is fixed), and we return a tz aware datetime in the response. But of course might be that others put the burden on the client. |
|
I can add support for Valhalla, no worries |
|
I see your point. I wanted to be explicit because API input and output formats are often very disparate. For example, OTP takes a local datetime as input and returns a timestamp (UTC). In this particular case, apart from specifying the timezone for the input, I don't see how you can know the local time for the output. By the way, I forgot to specify the timezone for the OTP input. |
khamaileon
force-pushed
the
departure-arrival-time
branch
from
cfcdc96
to
d37da63
Compare
|
@nilsnolde I committed a "naive" version of the feature. But with hindsight, I think the "aware" version is the best. If you want to use python datetime objects, they have to be timezone aware. Otherwise, it's better to use only UTC timestamps. |
khamaileon
force-pushed
the
departure-arrival-time
branch
from
f0665e6
to
66dfc50
Compare
khamaileon
force-pushed
the
departure-arrival-time
branch
from
66dfc50
to
e303406
Compare
| def _time_object_to_aware_datetime(self, time_object): | ||
| timestamp = time_object["value"] | ||
| dt = datetime.datetime.fromtimestamp(timestamp) | ||
| timezone = pytz.timezone(time_object["time_zone"]) | ||
| return dt.astimezone(timezone) |
There was a problem hiding this comment.
can we make these new methods util methods instead? I feel it's better to outsource those functions (here and otp) so they can be re-used. I'll do valhalla anyways and will PR to your PR, then you can see if you like that too:)
There was a problem hiding this comment.
This one in particular is specific to Google because the input format is theirs. Do we output to a Google-specific utility file?
There was a problem hiding this comment.
I tried to generalize it a bit in https://github.com/khamaileon/routingpy/pull/1/files
| self.client._request("/distancematrix/json", get_params=params, dry_run=dry_run) | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def parse_matrix_json(response): | ||
| def _parse_matrix_json(self, response): |
There was a problem hiding this comment.
hm why this change? this is "public" and staticmethod on purpose, we use this in other projects IIRC
There was a problem hiding this comment.
In this case, it prevented other class methods (_parse_legs) from being called. We'll have to figure out how to rewrite this.
There was a problem hiding this comment.
I did a small rewrite, none of those functions needed access to the instance, they can all be static actually
| def _timestamp_to_utc_datetime(self, timestamp): | ||
| dt = datetime.datetime.fromtimestamp(timestamp / 1000) | ||
| return dt.astimezone(datetime.timezone.utc) | ||
|
|
||
| def _parse_directions_response(self, response, num_itineraries): |
There was a problem hiding this comment.
that brings up another good point: this should be static and "public" to be consistent with the other interfaces. we do use this function type in other projects, but also it doesn't need anything from its instance, so it can be static.
There was a problem hiding this comment.
This one would fit nicely in a utils file because it's generic.
|
Oh yeah @khamaileon, I just realized you're not watching your routingpy fork (obviously, guess you didn't expect a issue/PR there..), but I did open this: khamaileon#1. I mentioned it above, but not very explicitly.. |
|
Oh yeah, I saw it, thanks. I'll try to take a look at it today! |
No description provided.