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

Should Lingua Franca have externally facing datetime conversion utilities? #182

Open
krisgesling opened this issue Mar 29, 2021 · 7 comments

Comments

@krisgesling
Copy link
Contributor

Currently there are a number of functions in lingua_franca.time that were brought along from mycroft-core when LF was extracted. Presently none of them are used in LF with the exception of using the default timezone particularly with PR #180.

These are really just short wrappers around datetime and dateutil.tz for getting relevant tzinfo.

They don't seem to fit within Lingua Franca's intended purpose of being a "multilingual formatting and parsing library".

As such I'm proposing that we remove the whole time module and shift the default tz methods that are used to lingua_franca.config.

This proposition is intended as an open invitation to say why they should be in there. If there are useful purposes that fit within the scope of the library then of course it should stay. It just doesn't seem like there are to me at the moment.

@JarbasAl
Copy link
Collaborator

timezones belong in a date parser library, if its returning and consuming dates why shouldn't there be utils to handle them?

they dont belong where they are used but belong in a config file? that makes even less sense to me

if someone wants a datetime in the tz configured for LF (assuming #180 gets merged) why can't we make that easy with the wrapper methods? again, should we force boilerplate for menial tasks we need to do internally either way? The other advantage is that whatever we decide in the future can be easily deprecated / improved without bothering users and forcing them to waste even more time in said boilerplate. if i want to extract a date, its not unlikely i want to also manipulate a date.

@ChanceNCounter
Copy link
Contributor

tbh I've been looking at mycroft.util.time as the oversight 😛 I'd just as soon move everything from mycroft.util that could be termed "localization" over to LF. But that's me.


time.default_timezone() is just an alias-wrapper for dateutil.tz.tzlocal(). This is definitely sugar for Skill devs. Can't argue there, but I don't think it makes more sense moving it back to Mycroft-core.

now_local() is internally useful as an anchor date. At minimum, it should stay.

The broader thing with TZs and LF, though, is that since LF needs to be TZ-aware, it might as well be able to report that information. LF has to "decide" what time zone it's in, if that information is not provided, and any ideal program would have a single source of truth for, "What time is it?"

Since LF could hypothetically disagree with datetime.datetime(), and will often disagree with API output, it should at least be possible for implementations to use LF as the single source of truth. The config option is always available to A) facilitate this or B) ignore it.

Having done that, the convenience methods now_local() and now_utc() seem like they should be public. to_local() and to_utc() are also just sugar.

@ChanceNCounter
Copy link
Contributor

OH, and, default_timezone() will no longer be a wrapper around dateutil! I dunno how I missed that. With the TZ-awareness fix, it should return the value from LF.config

At that point, it's still sugar, but it's our sugar.

lingua_franca.time.default_timezone() == lingua_franca.config.get("default_tz") or lingua_franca.config["timezone"] or something

This may or may not be worth having. It's certainly less confusing for "general users", as opposed to full integrations. That is, Mycroft-core will want to work with lingua_franca.config, but that's internal wiring. Skill devs, for instance, probably shouldn't touch LF's configs unless absolutely necessary. That's for the main implementation to change language and locale settings. Even when you wanna invoke a specific language, you don't do that by changing config, you do it with the loaders and lang= params.

@JarbasAl
Copy link
Collaborator

JarbasAl commented Mar 29, 2021

i agree with @ChanceNCounter that the time methods in core make more sense to be removed than in LF, its just duplicating code, and we cant really remove those from LF

mycroft.util.time should just become

from dateutil.tz import gettz
from lingua_franca.time import set_default_tz as _set_default_tz, now_utc, \
    now_local, to_local, to_utc, to_system, default_timezone as _default_tz


def set_default_tz(tz=None):
    _set_default_tz(tz if tz else default_timezone())


def default_timezone():
    """Get the default timezone
    Based on user location settings location.timezone.code or
    the default system value if no setting exists.
    Returns:
        (datetime.tzinfo): Definition of the default timezone
    """
    try:
        # Obtain from user's configurated settings
        #   location.timezone.code (e.g. "America/Chicago")
        #   location.timezone.name (e.g. "Central Standard Time")
        #   location.timezone.offset (e.g. -21600000)
        from mycroft.configuration import Configuration
        config = Configuration.get()
        code = config["location"]["timezone"]["code"]

        return gettz(code)
    except Exception:
        # Just go with system default timezone
        return _default_tz()

@krisgesling
Copy link
Contributor Author

To me it comes back to what we want LF to be.

With datetime utilities as a public facing tool it becomes: "multilingual parsing and formatting, and timezone conversions".

Trying to make it a general "localization" library raises the question of where do we stop? What is out of scope? Translations?

@ChanceNCounter
Copy link
Contributor

I think it's more, "Multilingual parsing and formatting, with locale-awareness." At a certain level, the entire module is about where you are, or where the listener is. BCP language codes are familiar to most end users from keyboard mappings.

I'm not as interested in providing timezone conversions as the fact that we might as well expose the ones we have. And, having done that, we're providing half of these funcs, whereas the other half could be shrugged off as out of scope and left in Mycroft. But they're sides of the same coin. If we need some of these funcs internally, why not just migrate them, and if we've migrated some of them, why not migrate the others? They're all just sugar.

Otherwise, I think we're pretty much where we stop. Lingua Franca: What day is it, what is five times three, and how long until midnight, in the language of my choosing?

@ChanceNCounter
Copy link
Contributor

Put differently, LF ate mycroft.util.lang, and I'd be happy for it to eat mycroft.util.time, but I can't think of anything else it should get. Only new functions that would've gone in those not-modules.

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

No branches or pull requests

3 participants