Add configurable default timezone information #197

krisgesling · 2021-05-28T07:42:05Z

Description

Adds configurable default timezone information for use by

Default tz-info can be set with lingua_franca.time.set_default_tz
All datetime extractors default to now_local for anchorDate
All localized functions now inject a tzinfo in all tz-naive datetime objects passed as arguments. Note tz-naive objects are presumed to be in UTC as there is no perfect answer here.
- Note this will also be done on methods that do not explicitly require tzinfo such as nice_time()
- As such projects are encouraged to pass in timezone aware datetime objects where ever possible.
Timezone info injection can be enabled/disable in lingua_franca.config (default: enabled)
Lingua Franca should always return a timezone aware datetime object in the configured default timezone

Extends and replaces PR #180 - squashed commits and fixed failing tests.

Type of PR

Bugfix
Feature implementation

Testing

Unit tests have been updated.

ChanceNCounter · 2021-06-02T18:38:37Z

Sorry, lost in notification spam. Tagging for review.

ChanceNCounter · 2021-06-03T20:27:54Z

I would like to clarify whether the Czech datetime extractor is injecting tzinfo in all the scenarios where @JarbasAl's logic does so in the other files.

lingua_franca/lang/parse_es.py

lingua_franca/lang/parse_fr.py

Tony763 · 2021-06-04T04:34:32Z

I would like to clarify whether the Czech datetime extractor is injecting tzinfo in all the scenarios where @JarbasAl's logic does so in the other files.

Hi, @ChanceNCounter when I did a Czech support, I took a English files and did 1:1 copy. If You need to change something, write what an I will look into it. 🙂

ChanceNCounter · 2021-06-04T19:57:06Z

lingua_franca/internal.py

+            # Check if we need to add timezone awareness to any datetime object
+            if config.inject_timezones:
+                for key, value in kwargs.items():
+                    if isinstance(value, datetime) and value.tzinfo is None:
+                        kwargs[key] = to_local(value)
+                for idx, value in enumerate(args):
+                    if isinstance(value, datetime) and value.tzinfo is None:
+                        args = args[:idx] + (to_local(value),) + args[idx + 1:]


@krisgesling I am confused by this tuple's tupleyness

Yeah I did a double take on it at first too.
('a') provides a string; whilst
('a',) provides a single item tuple

I've switched it out to use iterable unpacking:

args = (*args[:idx], to_local(value), *args[idx + 1:])

hopefully this is more readable, though I also don't see it being used a lot so may still confuse some people.

It's significantly more readable, in that I thought the outer type was a list, but now I'm wondering why args is a tuple 👅

ChanceNCounter · 2021-06-04T19:59:49Z

@Tony763 I see that now, thanks! I was just as confused by the snippet's absence from the English parser, but I've checked, and all paths add tzinfo to that temp var.

ChanceNCounter · 2021-06-04T20:05:09Z

Apart from the remaining question on internal.py, I'd like to test that valid tzinfo isn't being overwritten.

JarbasAl · 2021-06-09T07:04:13Z

test/test_format.py

-        dt = datetime.datetime(2017, 1, 31,
-                               13, 22, 3)
+        dt = datetime.datetime(2017, 1, 31, 
+                               13, 22, 3, tzinfo=default_timezone())


small note, i think its good to be explicit here, but passing this is fully optional, part of what this PR does is handle this transparently

the tests only need tzinfo added in returned results, all input dates will get it appended internally

in a sense this last commit adding all those tzinfos in optional places is now removing the testing for the functionality added by this PR

as i said i think it's good to be explicit in what is being tested, this comment is merely informative, not a request for a change

Yeah it is an issue I think.

With the naive datetimes having UTC tzinfo added, from memory the tests will fail if that's not your configured default unless you explicitly pass an aware object. So it is a bit of a patch job - we really need more detailed testing that checks those different scenarios.

krisgesling · 2021-06-09T10:39:32Z

I've just added an attempted test for using an explicitly different timezone. However it also suggests that one of the dot points from the PR description is false:

Lingua Franca should always return a timezone aware datetime object in the configured default timezone

This is also what the docstring says, but it possibly should be:

Lingua Franca should always return a timezone aware datetime object. If the method accepts a datetime object as an argument, and a timezone aware object is passed, the return value will be in the same timezone. If there is no datetime object argument, or a timezone naive object is passed, the configured default timezone will be used.

ChanceNCounter · 2021-06-09T15:27:15Z

I agree, and had been reading,

always return a timezone aware datetime object in the configured default timezone [if LF had to add the tzinfo itself]

We definitely don't want to clobber tz-aware datetimes back to the configured tz!

ChanceNCounter

Thought I pressed approve on my previous comment. Whoops =)

- Switch None checks from == to is - Add default tzinfo to tests - remove stray commented code - improve readability of tuple - add test for extracting datetime in non-default timezone

krisgesling · 2021-06-09T20:42:22Z

What about we just remove the word "user's":

[:obj:datetime, :obj:str]: 'datetime' is the extracted date
as a datetime object in the user's local timezone.

becomes

[:obj:datetime, :obj:str]: 'datetime' is the extracted date
as a datetime object in the local timezone.

ChanceNCounter · 2021-06-10T00:36:56Z

That'd work.

devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label May 28, 2021

ChanceNCounter self-requested a review June 2, 2021 18:38

ChanceNCounter reviewed Jun 3, 2021

View reviewed changes

lingua_franca/lang/parse_es.py Outdated Show resolved Hide resolved

ChanceNCounter reviewed Jun 3, 2021

View reviewed changes

lingua_franca/lang/parse_fr.py Show resolved Hide resolved

ChanceNCounter reviewed Jun 4, 2021

View reviewed changes

ChanceNCounter linked an issue Jun 4, 2021 that may be closed by this pull request

Should we require that all datetime params are tz aware #183

Closed

JarbasAl reviewed Jun 9, 2021

View reviewed changes

ChanceNCounter approved these changes Jun 9, 2021

View reviewed changes

JarbasAl and others added 2 commits June 9, 2021 11:37

feat/timezone_config

86a734a

Tests and touchups

63ece83

- Switch None checks from == to is - Add default tzinfo to tests - remove stray commented code - improve readability of tuple - add test for extracting datetime in non-default timezone

ChanceNCounter force-pushed the feature/default-tz branch from b5fbb29 to 63ece83 Compare June 9, 2021 18:39

ChanceNCounter approved these changes Jun 9, 2021

View reviewed changes

ChanceNCounter merged commit 3e66af9 into master Jun 9, 2021

ChanceNCounter mentioned this pull request Aug 16, 2021

extract_datetime_de fails when anchor is None #194

Closed

ChanceNCounter linked an issue Aug 16, 2021 that may be closed by this pull request

extract_datetime_de fails when anchor is None #194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configurable default timezone information #197

Add configurable default timezone information #197

krisgesling commented May 28, 2021

ChanceNCounter commented Jun 2, 2021

ChanceNCounter commented Jun 3, 2021

Tony763 commented Jun 4, 2021 •

edited

Loading

ChanceNCounter Jun 4, 2021

krisgesling Jun 9, 2021

ChanceNCounter Jun 9, 2021

ChanceNCounter commented Jun 4, 2021

ChanceNCounter commented Jun 4, 2021

JarbasAl Jun 9, 2021

krisgesling Jun 9, 2021

krisgesling commented Jun 9, 2021

ChanceNCounter commented Jun 9, 2021

ChanceNCounter left a comment

krisgesling commented Jun 9, 2021

ChanceNCounter commented Jun 10, 2021

Add configurable default timezone information #197

Add configurable default timezone information #197

Conversation

krisgesling commented May 28, 2021

Description

Type of PR

Testing

ChanceNCounter commented Jun 2, 2021

ChanceNCounter commented Jun 3, 2021

Tony763 commented Jun 4, 2021 • edited Loading

ChanceNCounter Jun 4, 2021

Choose a reason for hiding this comment

krisgesling Jun 9, 2021

Choose a reason for hiding this comment

ChanceNCounter Jun 9, 2021

Choose a reason for hiding this comment

ChanceNCounter commented Jun 4, 2021

ChanceNCounter commented Jun 4, 2021

JarbasAl Jun 9, 2021

Choose a reason for hiding this comment

krisgesling Jun 9, 2021

Choose a reason for hiding this comment

krisgesling commented Jun 9, 2021

ChanceNCounter commented Jun 9, 2021

ChanceNCounter left a comment

Choose a reason for hiding this comment

krisgesling commented Jun 9, 2021

ChanceNCounter commented Jun 10, 2021

Tony763 commented Jun 4, 2021 •

edited

Loading