-
Notifications
You must be signed in to change notification settings - Fork 60
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
my.time.tz: implement different policies for localizing #106
Conversation
my/time/tz/common.py
Outdated
# todo 'warn'? not sure if very useful | ||
] | ||
# todo make the default configurable, e.g. add my.cofnig.time.tz section | ||
DEFAULT_POLICY: Policy = 'keep' |
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.
not sure what the default should be, 'keep' or 'convert'? @seanbreckenridge what do you think?
I'm not sure because I'm expecting to use this in 'error' mode most of the time to fail fast :)
An upside of convert
would be is that you basically put trust into the tz
module, which is ultimately good. A downside is that it might be too coarse (e.g. if tz
fell back on the home
locations), in that case the original timezone might be better.
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 assume this will will be configurable through my.config
. I think it makes sense to have the default be keep
, forcing the user to be aware of the tz
module and configure it before it possibly makes changes to the data they may not be expecting.
- it's safer when either all of your objects are tz aware or all are tz unware, not a mixture | ||
- you might trust your original tiemzone, or it might just be UTC, and you want to use something more reasonable | ||
''' | ||
Policy = Literal[ |
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.
Not sure about Literal
, haven't really used it much before.. But in my experience having real enums for such things just makes it a bit annoying (e.g. you have to import it first, etc). I guess mypy gives enough safety while keeping it flexible.
Either way should be easy to switch in the future, so maybe good to give it a go anyway.
f8c5989
to
b731872
Compare
wonderful 👍 In [1]: from more_itertools import last
In [2]: from my.time.tz.main import localize
In [3]: from my.body import weight
In [4]: localize(last(weight()).when)
Out[4]: datetime.datetime(2020, 10, 31, 15, 18, 57, tzinfo=<DstTzInfo 'America/Los_Angeles' PDT-1 day, 17:00:00 DST>) Not sure if you want to add this as a utility function to HPI directly, but I'll probably add a hacky function like: X = TypeVar("X")
def localize_iterable(it: Iterator[X]) -> Iterator[X]:
"""
If policy == convert, localize all datetimes on some iterator
"""
if policy != 'convert':
yield from it
return
for item in it:
if hasattr(it, 'dt'):
# tz.localize and yield
elif hasattr(it, 'at'):
# tz.localize and yield
elif hasattr(it, 'when'):
# tz.localize and yield
else:
warnings.warn(f"Couldnt find datetime attr {it}")
yield tz Seeing as I have multiple attributes that match datetimes, and I prefer 'at'/'when' to 'dt' sometimes. May be easier to standarize 'dt' across all modules (or use |
Oh, nice! Good idea, I'll think about it too.
Yeah, or a special decorator, or type alias (which acts like Also this feels like it's begging for lenses :) Haven't really used them in other languages before either, but found something for Python https://github.com/ingolemo/python-lenses |
related: #17
also follow up for #96 (comment)