Add dateutil.utils.today #474
Conversation
| """ | ||
|
|
||
| dt = datetime.now(tzinfo) | ||
| return datetime.combine(dt.date(), time(0, tzinfo=tzinfo)) |
There was a problem hiding this comment.
Responding to this comment by @jbrockmendel:
+1 for today(tzinfo), but I'm very wary of implementing behavior that differs from same-named stdlib behavior. btw:
>>> tz = pytz.timezone('US/Pacific') >>> pd.Timestamp.today(tz) Timestamp('2017-10-14 17:19:04.081315-0700', tz='US/Pacific')
My reason for calling it today was because, frankly, I think that this is close to datetime.datetime.today() should do, and I think it violates principle of least surprise to find that datetime.today() is basically just an alias for datetime.now(). That said, the name overloading is my main concern about this function.
I think there are a few reasonable ways forward:
- Leave as is because this is what
datetime.today()should have been and people are more likely to be shocked thatdatetime.datetime.today()does something else than thatdateutil.utils.today()does what it does. - Change the signature of this so that
utils.today()takes an optionaltimeargument in addition to an optionaltzinfoargument. This makes it more clear that what it does is give you "today at this time, defaulting to midnight". The reason I don't like this is that there could be a conflict betweentime(which may have atzinfoof its own) andtzinfo, and also the interface isn't nearly as clean. - Change the name to something like
today_midnight()ortoday_datetime()or something.
I think I'm inclined to leave it as is, considering pendulum has a similar real distinction between its now and today behavior:
>>> import pendulum
>>> print(pendulum.now())
2017-10-15T17:30:33.887480-04:00
>>>> print(pendulum.today())
2017-10-15T00:00:00-04:00| package_data={"dateutil.zoneinfo": ["dateutil-zoneinfo.tar.gz"]}, | ||
| zip_safe=True, | ||
| requires=["six"], | ||
| tests_require=["freezegun"], |
There was a problem hiding this comment.
I should note that I'm not sure that this is the most foolproof way of installing test-specific dependencies, as the only thing that cares about this particular line is, AFAIK, python setup.py test, and if we're going to move to pytest, we'll either want our own setup.py test runner that uses pytest, and/or we should have a separate script that declares and installs the test dependencies independent of python setup.py test. Not sure what most people do.
|
I'm inclined to agree about the ideal behavior of
Note there is a small difference: If this is intended to be user-facing, then the name should avoid adding to the morass here. If it is intended to be internal, I would be fine with |
|
@jbrockmendel It's definitely intended to be external-facing. I honestly don't see a big problem with diverging from
Wow, I didn't even realize this. So it's basically a worse version of |
|
The downside of name overlap strikes me as much bigger than the upside from a one-liner. Since we'll be stuck with the API indefinitely, a distinct name seems like the way to go. That said, its not worth arguing about, so do your thing. |
|
I don't really think there's a serious downside from the name overlap. It's not even possible to import I'm really torn between:
So far I'm still liking |
|
OK, I'm going to go ahead and merge this. I doubt anyone will be too confused that |
This seems like a convenience function that would be useful, particularly for passing in to
parser.parseasdefault.