briancurtin / deprecation

A library to handle automated deprecations
Apache License 2.0
88 stars 31 forks source link

Allow removed_in to be a date #46

Closed gabbard closed 4 years ago

gabbard commented 4 years ago

Depending on the versioning scheme used by a project, it can be difficult to tell in advance the version number at which code can be removed. Sometimes, though, it is easier to tell when it should be removed. Google Guava, for example, guarantees all methods marked deprecated will be kept around for two years post-deprecation.

It would be convenient to handle this by allowing a date (in some TBD format) to be passed for the removed_in argument.

briancurtin commented 4 years ago

I like this idea. I would think if you set it to a datetime.date that would be sufficient and unambiguous, right? It would be converted to that anyway, and makes the handling difference between a date and a version string a bit more clear.

gabbard commented 4 years ago

@briancurtin : Yes, I agree that datetime.date would be a good choice.

gabbard commented 4 years ago

One small wrinkle: should we require the user to specify tzinfo on the datetime.dates? If not, what should be our default behavior - to use UTC or the user's local timezone?

The latter could easily confuse the user ("why is this triggering as removed already when I have the date set to tomorrow?"), but the former could lead to weird situations when the code is executing in multiple timezones (e.g. the CI server is in a different timezone than the developer, so the tests start failing in CI while succeeding locally or vice-versa).

gabbard commented 4 years ago

@briancurtin : if it is okay with you, this could be a good exercise for @aakum03 (an undergraduate co-op student working with me this semester). I'm happy to do the first passes of review for him before you need to take a look.

briancurtin commented 4 years ago

One small wrinkle: should we require the user to specify tzinfo on the datetime.dates? If not, what should be our default behavior - to use UTC or the user's local timezone?

The latter could easily confuse the user ("why is this triggering as removed already when I have the date set to tomorrow?"), but the former could lead to weird situations when the code is executing in multiple timezones (e.g. the CI server is in a different timezone than the developer, so the tests start failing in CI while succeeding locally or vice-versa).

I would think we take a date in the removed_in parameter, and that gets compared against date.today() which is local to the runtime. Neither of those take or otherwise work with a tzinfo unless we get into datetimes where we only care about the date part, but I think that probably overcomplicates the whole thing to be just slightly more accurate in when a test fails.

I think explicitly documenting that removed_in being set to a date compares that date to date.today()—noting that this function is local—would be better than deprecated(...) needing to grow a parameter for tzinfo that only matters in the case of removed_in being a date object. That's a little too specialized for me and only helps one case be slightly more accurate in a test scenario.

gabbard commented 4 years ago

@briancurtin : Good points - I had missed that only datetime.datetime has tz_info and not datetime.date. I've gotten spoiled by Java's time APIs. :-)

gabbard commented 4 years ago

Great - thanks @aakum03 for implementing this, and thanks @briancurtin for being so responsive about merging it!