Timezones #4460
Timezones #4460
Conversation
Display relies on the template group 'list'.
Time in TimezonesDescription: TimezoneTime IA will give time in any respective time zone. Example Query: time in ist, time est, time in bst Tab Name: Source: These are the important fields from the IA page. Please check these for errors or missing information and update the IA page This is an automated message which will be updated as changes are made to the IA page |
lib/DDG/Goodie/Timezonetime.pm
Outdated
|
|
||
| # Get time for desired timezones | ||
| my $dt = DateTime->now(time_zone => 'UTC'); | ||
| my @times = map { |
There was a problem hiding this comment.
I think this might be clearer as a for loop rather than a map, with the multiple statements within it's pretty difficult to read
There was a problem hiding this comment.
Hmm also it might be worth building this outside of the handle so we can just just lookup the answer?
There was a problem hiding this comment.
I'll rewrite it using a for loop with a push inside.
Yes, we can convert offsets only once for each timezone. I'll fix this too.
Rewrite map into a for loop.
| { name => "Cuba Standard Time", | ||
| offset => "-5:00", | ||
| time => "04:36:53" }], | ||
| "CST") | ||
| ); | ||
|
|
||
| # INFO: Freezes time for below tests. | ||
| set_fixed_time('2016-01-03T09:36:53Z'); | ||
|
|
||
| ddg_goodie_test( | ||
| ['DDG::Goodie::Timezonetime'], |
There was a problem hiding this comment.
I'd like some tests here to prove the behaviour when DST is in effect, that's the purpose of the set_fixed_time
There was a problem hiding this comment.
Right now, DST doesn't impact the behaviour because the offset is fixed. Only the abbreviation changes, for instance from 'cet' to 'cest'. I may have missed something but I don't understand what you mean exactly.
There was a problem hiding this comment.
Basically as this is a time-based IA I'd like to have tests to assert the behaviour when the IA is executed at different times of year, does that make sense? @gargaml
There was a problem hiding this comment.
Definitely 👍 I'm going to fix this.
|
@gargaml @mintsoft sorry for getting to this late. I think the abbreviations work is good, however I don't agree with the decisions to disambiguate the abbreviations. In almost all of these cases, one of the abbreviations is much more popular. IMO we should only show the most popular result. This keeps the design simple and easy to understand. For example PST shows Pacific Time, and Pitcairn. The latter of which I'm sure 99% of users won't know or care about. What's more important for this IA is understanding when DST is in effect and showing the user the correct DST time even when they don't specifically ask for it. Google does a great job on all of the above. They don't disambiguate (but they do accept more specific queries i.e. https://encrypted.google.com/search?hl=en&q=pitcairn%20time) They also show ET for EST or EDT and always show the DST time. This is what we should be focusing on here. @gargaml if you can get this done in the next two weeks that'd be great. Now that DDH is in maintenance mode we'll be closing out open PRs that are not essential bug fixes. This is not considered an essential PR, so I'd like to get it wrapped up asap. |
|
Also, the list view isn't great when there are no disambiguations:
The time is the answer here, but it is the 3rd part the eye sees, because we read top -> bottom, left->right. It should be the most prominent part of the answer. We should make the time the
Also, we can drop the seconds from the time. |
|
Google doesn't process every timezones in the same way. The case ('et', 'est', 'edt') shows the same time but the case ('cet', 'cest') behaves differently and doesn't consider DST. When the user enters 'et', I agree that we should show a result which considers DST because it's an abbreviation of an abbreviation. That's why 'et' is not part of the yaml file. I'm going to remove less popular abbreviations. |
@gargaml good catch, I hadn't noticed that. They seem to consider DST for PST, CST, MST, EST. Oddly they don't for AST/ADT. I think we can mimic their behaviour because it's what most users will be familiar with. |
|
@gargaml Ping! Would you be able to finish this one up? We'd love to get this live! If you can get it done today I'll be able to merge. Otherwise I'll be closing this until it becomes a priority for us again. Thanks for all the work so far! |
Description of new Instant Answer, or changes
This pull request adds some abbreviations support to the timezone instant answer. The main issue comes from ambiguities with respect to abbreviations. For instance, BST stands for British Standard Time, Bangladesh Standard Time and Bougainville Standard Time. Instead of choosing one based on arbitrary criteria, we rely on the template group 'list' to provide several answers as needed.
To do this, we add a new file (abbreviations.yaml) to get timezone names corresponding to the abbreviation used in the query.
Related Issues and Discussions
It relates to this discussion duckduckgo/zeroclickinfo-spice#3181
This is just a proof of concept as it doesn't consider daylight saving time (as the previous version did). I just consider that a timezone exists throughout the year. Only countries can change of timezone for DST.
People to notify
@pjhampton, @moollaza, @shivam99aa
Testing & Review
To be completed by Community Leader (or DDG Staff) when reviewing Pull Request
Pull Request
Instant Answer Page (for new Instant Answers)
Code
$ duckpan test <goodie_id>)Ready to merge?
Pull Request Review Guidelines: https://docs.duckduckhack.com/testing-reference/pr-review.html
Instant Answer Page: https://duck.co/ia/view/timezonetime