Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add departure and arrival datetime to Direction #116

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

khamaileon
Copy link
Collaborator

No description provided.

@khamaileon khamaileon force-pushed the departure-arrival-time branch 3 times, most recently from 0d73dd1 to 4886c83 Compare August 3, 2023 11:02
@khamaileon
Copy link
Collaborator Author

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.

@nilsnolde
Copy link
Owner

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.

@nilsnolde
Copy link
Owner

I can add support for Valhalla, no worries

@khamaileon
Copy link
Collaborator Author

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 khamaileon closed this Aug 3, 2023
@khamaileon khamaileon reopened this Aug 3, 2023
@khamaileon khamaileon closed this Aug 3, 2023
@khamaileon khamaileon reopened this Aug 3, 2023
@khamaileon
Copy link
Collaborator Author

@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.

Copy link
Owner

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! I realized that we should make some functions "public". Like I mentioned above, I'll PR to your PR with the valhalla changes, then I can show what I'd do.

Comment on lines +329 to +333
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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one in particular is specific to Google because the input format is theirs. Do we output to a Google-specific utility file?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm why this change? this is "public" and staticmethod on purpose, we use this in other projects IIRC

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, it prevented other class methods (_parse_legs) from being called. We'll have to figure out how to rewrite this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same for the isochrones

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one would fit nicely in a utils file because it's generic.

@nilsnolde
Copy link
Owner

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..

@khamaileon
Copy link
Collaborator Author

Oh yeah, I saw it, thanks. I'll try to take a look at it today!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants