-
Notifications
You must be signed in to change notification settings - Fork 673
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
refactor datetime output #4365
refactor datetime output #4365
Conversation
src/baldr/datetime.cc
Outdated
std::string | ||
offset_date(const std::string& in_dt, const uint32_t in_tz, const uint32_t out_tz, float offset) { | ||
// get the input UTC time, add the offset and translate to the out timezone | ||
auto iepoch = DateTime::seconds_since_epoch(in_dt, DateTime::get_tz_db().from_index(in_tz)); | ||
auto oepoch = | ||
static_cast<uint64_t>(static_cast<double>(iepoch) + static_cast<double>(offset + .5f)); | ||
return DateTime::seconds_to_date(oepoch, DateTime::get_tz_db().from_index(out_tz), false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I broke it up a bit, so we can put this here in baldr/date_time without including GraphReader too. I added a function to GraphReader instead.
int GraphReader::GetTimezoneFromEdge(const baldr::GraphId& edge, graph_tile_ptr& tile) { | ||
auto nodes = GetDirectedEdgeNodes(edge, tile); | ||
if (const auto* node = nodeinfo(nodes.first, tile)) | ||
return node->timezone(); | ||
else if (const auto* node = nodeinfo(nodes.second, tile)) | ||
return node->timezone(); | ||
|
||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
analogous to GraphReader::GetTimezone for nodes
static_cast<uint64_t>(time)); | ||
float time = connection.cost.secs; | ||
// no datetime for unfound connections | ||
if (time < kMaxCost) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't add date_time when there's no connection, but we do add it for origin == target. in the serializer we put null
when there was no connection
4095496
to
add7b25
Compare
valhalla/thor/matrix_common.h
Outdated
if (pred_id.Is_Valid()) { | ||
// get the timezone of the output location | ||
auto out_nodes = reader.GetDirectedEdgeNodes(pred_id, tile); | ||
dest_tz = reader.GetTimezone(out_nodes.first, tile) || reader.GetTimezone(out_nodes.second, tile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized, this PR will also fix #4360 . this line is not what python-eyes think it is.. dest_tz
will be true or false, i.e. 1 or 0. And 1 must be UTC. The new logic doesn't have that flaw.
I drafted this bcs we agreed to output the timezone info as an extra response field. I’d just put the „+02:00“ in an extra field. |
This is ready. In my last comment I must've confused the PR. This is a pure refactor (plus tiny bug fix), adding tz info would be in a next PR. So this is ready for final review. |
* @param tile Current tile. | ||
* @return Returns the timezone index. A value of 0 indicates an invalid timezone. | ||
*/ | ||
int GetTimezoneFromEdge(const baldr::GraphId& edge, graph_tile_ptr& tile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just need to check and mention if the tile is modified. if its not modified we should pass it wihtout the reference if modified we should say whether its for the end or begin node of the edge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, in nodeinfo()
the tile can change, so we could end up with the edge's end node's tile instead. is a comment in the docstring good enough for you or you want smth more elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah just docstring update was all i wanted. i hope that its a gaurantee though that as a post condition it will be the endnodes tile not just could be, i guess when the tile isnt found it could be, thats ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, our own usage of this (so far) is safe, we don’t even use the tile, only for caching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep yep its just to avoid mistakes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i hope that its a gaurantee though that as a post condition it will be the endnodes tile not just could be, i guess when the tile isnt found it could be, thats ok.
after this function is called, it's very unlikely that the tile is changed (unless you compiled the graph without timezone info, but maybe that's even handled further up in the processing chain). that only happens if the edge's start node has no timezone info.
the history and subsequently the diff of this branch are screwed up. |
fixed. btw, regarding your comment if the tile is being switched in the new |
@chrstnbwnkl will PR tmrw or so with the next bit to actually return the tz offset in the response:) |
Route & matrix didn't share the code to generate the response's datetime strings. Can't remember why I didn't do that in #4071. I thought I'll be smarter with my PRs for once, and not cram refactors together with functional changes:) Comes from the experience of reviewing such PRs I guess (not pointing fingers!).
Really, it's to prepare for outputting timezone-aware datetime strings: #4241. Last chance to tell me that no one wants that:) It's a breaking change the way I'd do it (i..e. just return
2023-02-27T22:50+02:00
instead of2023-02-27T22:50
). Depending on the client code that could be a problem.. IMO it's acceptable.EDIT: Also fixes #4360 as collateral damage..