khal icon indicating copy to clipboard operation
khal copied to clipboard

Update khal to Follow PEP495 Removing Deprecation Warnings

Open pjkaufman opened this issue 2 years ago • 35 comments

When running khal ... or ikhal ... an issue would arise where warnings about deprecated functions being used (see https://github.com/pimutils/khal/issues/1092). This was not a critical change that was needed, but it was more to remove the warnings as an eye sore for myself and those that would not be able to do so on their own.

Changes Made:

  • Updated AUTHORS.txt as per the documentation for contributing
  • Imported and used ZoneInfo in order to assign a timezone where the warnings were being thrown (2 locations in khalendar.py and event.py though I decided to add two more locale format updates in one of the methods here)

I had to convert the timezone info to a string just in case it was something else because when I did not do so, I got errors. Also, I did initially try to change all of the localize methods to use ZoneInfo, but that started causing test cases to fail for certain files, so I opted to just fix the visible issue.

I am newish to python, but have used it to write programs or scripts before. So please let me know if there is something I missed, a better way to do things, or if this PR should be more extensive.

Here is the source I am using for determine how to convert code to the PEP495: https://pytz-deprecation-shim.readthedocs.io/en/latest/migration.html

Note: there is no longer a built in list of transition dates for daylight savings times, so it will need to be decided how to handle this especial since the standards for this have changed several times.

pjkaufman avatar Oct 22 '21 12:10 pjkaufman

Correct me if I am wrong, but it looks like pre-commit.ci has not been added to the actual ci yet since the PR is currently pending a merge. Is there a way I should modify my branch to account for this, or do I need to wait on that other PR which has been waiting for approval for 2 months?

Thank you for letting me know!

pjkaufman avatar Oct 22 '21 12:10 pjkaufman

Note: I did miss the khal at method and need to check and see what needs to be done for that.

pjkaufman avatar Oct 22 '21 12:10 pjkaufman

Note: I did miss the khal at method and need to check and see what needs to be done for that.

This was taken care of earlier. I am now just waiting on guidance regarding the pre-commit issue.

pjkaufman avatar Oct 22 '21 16:10 pjkaufman

I just realized that there are other places where these warnings are displayed as well. I am going to try to fix as many of them as I can, but this may be above my understanding level. The timezone change is simple enough, but the problem seems to arise when dealing with all of these different icalendar attributes and how the new way of doing these things should be done in the current standard.

pjkaufman avatar Oct 23 '21 02:10 pjkaufman

@WhyNotHugo , PEP495 no longer has transition dates for when going into or out of daylight savings (https://pytz-deprecation-shim.readthedocs.io/en/latest/migration.html#semantic-differences-between-is-dst-and-fold). How would we like to handle that?

Possible Solutions:

  1. I can try to estimate the DST days for years since 2007 using an algorithm for the standards set then (https://www.timeanddate.com/time/change/usa)
  • We could extend this to other date ranges as needed
  1. we can hard code our own list of when DST happened in each year

Also BST would need a similar solution.

What do you think about this?

pjkaufman avatar Oct 23 '21 15:10 pjkaufman

Though I would think suppressing dependency print output would probably be bad practice.

Agreed.

Warnings in dependencies need to be addressed in the dependencies themselves. Problem is, some of our dependencies are growing stale or not supporting modern pythons.

WhyNotHugo avatar Oct 23 '21 15:10 WhyNotHugo

On Sat, 23 Oct 2021, at 17:37, Peter Kaufman wrote:

@WhyNotHugo https://github.com/WhyNotHugo , PEP495 no longer has transition dates for when going into or out of daylight savings (https://pytz-deprecation-shim.readthedocs.io/en/latest/migration.html#semantic-differences-between-is-dst-and-fold). How would we like to handle that?

Possible Solutions:

  1. I can try to estimate the DST days for years since 2007 using an algorithm for the standards set then (https://www.timeanddate.com/time/change/usa)
  • We could extend this to other date ranges as needed
  1. we can hard code our own list of when DST happened in each year Also BST would need a similar solution.

What do you think about this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pimutils/khal/pull/1093#issuecomment-950169788, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFSNOZECOMZJE5LDQMJRLTUILJENANCNFSM5GQNTTHA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Sorry, I'm not fully understanding here. What exactly is it that's missing on PEP495?

WhyNotHugo avatar Oct 23 '21 15:10 WhyNotHugo

On Sat, 23 Oct 2021, at 17:37, Peter Kaufman wrote: @WhyNotHugo https://github.com/WhyNotHugo , PEP495 no longer has transition dates for when going into or out of daylight savings (https://pytz-deprecation-shim.readthedocs.io/en/latest/migration.html#semantic-differences-between-is-dst-and-fold). How would we like to handle that? Possible Solutions: 1. I can try to estimate the DST days for years since 2007 using an algorithm for the standards set then (https://www.timeanddate.com/time/change/usa) * We could extend this to other date ranges as needed 1. we can hard code our own list of when DST happened in each year Also BST would need a similar solution. What do you think about this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1093 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFSNOZECOMZJE5LDQMJRLTUILJENANCNFSM5GQNTTHA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. Sorry, I'm not fully understanding here. What exactly is it that's missing on PEP495?

@WhyNotHugo The issue is located here: https://github.com/pimutils/khal/blob/master/khal/khalendar/event.py#L856-L901 The first issue is that we are using the transition times defined by pytz which no longer exist when the timezone passed in is a ZoneInfo. Here is where we get that information currently: https://github.com/pimutils/khal/blob/master/khal/khalendar/event.py#L866-L868. Then we use _tzinfos to help determine our RDATE and timezone information for our events. I am not sure what the equivalent is in PEP495 so we might not be able to use the current system for determining BST and DST. If we want to fix this, we could have to swap over to a new way of doing things.

The nice thing is that time additions to a datetime will show a change in fold, but will not help us with our checks in this are of code (at least not that I am aware of).

Does that help clarify the issue?

pjkaufman avatar Oct 23 '21 20:10 pjkaufman

I think I have something for DST times that are newer than 2007 (I think I can add other date ranges as well, it is just that the algorithm for doing so will need to be modified. I am having issues with DST transition times. Is there a way outside of pytz to do determine its transition times?

Here is what I am thinking for the new create_timezone method:

def create_timezone(tz, first_date=None, last_date=None):
...
    # TODO last_date = None, recurring to infinity

    first_date = dt.datetime.today() if not first_date else to_naive_utc(first_date)
    # TODO: determine if there is a way to do this for BST pjk
    # check to see if timezone is a part of dst by checking if before or after the transition it has an offset
    if first_date.dst() == dt.timedelta(0) and first_date.replace(fold= 1).dst() == dt.timedelta(0):
        return _create_timezone_static(tz)
    
    last_date = dt.datetime.today() if not last_date else to_naive_utc(last_date)
    timezone = icalendar.Timezone()
    timezone.add('TZID', tz)

    transition_times = _get_transition_dates_dst_for_daterange(tz, first_date.replace(tzinfo=tz), last_date.replace(tzinfo=tz))
    timezones = {}
    for index in range(1, len(transition_times)):
        name = transition_times[index].tzname()
        if name in timezones:
            ttime = transition_times[index].replace(tzinfo=None)
            if 'RDATE' in timezones[name]:
                timezones[name]['RDATE'].dts.append(
                    icalendar.prop.vDDDTypes(ttime))
            else:
                timezones[name].add('RDATE', ttime)
            continue

        if transition_times[index].dst() != dt.timedelta(0):
            subcomp = icalendar.TimezoneDaylight()
        else:
            subcomp = icalendar.TimezoneStandard()
        
        subcomp.add('TZNAME', name)
        subcomp.add(
            'DTSTART',
            tz.fromutc(transition_times[index]).replace(tzinfo=None) # Note: for some reason this is using EDT instead of the zone info name for the value here, I need to figure out why as this is incorrect
        )
        
        subcomp.add('TZOFFSETTO', transition_times[index].utcoffset())
        subcomp.add('TZOFFSETFROM', transition_times[index - 1].utcoffset())
        timezones[name] = subcomp

    for subcomp in timezones.values():
        timezone.add_component(subcomp)

    return timezone

Here is the DST algorithm (this is more a proof of concept than an actual final algorithm):

def _get_transition_dates_dst_for_daterange(tz, start_date, end_date):
    #TODO: add date range based algorithms for determining the correct dst time zones where rules exist pjk
    if start_date.year < 2007:
        msg = (
                f"Cannot work with start dates prior to 2007 at this time. Date supplied was" + str(start_date) + "."
            )
        logger.fatal(msg)
        raise FatalError(  # because in ikhal you won't see the logger's output
            msg
        )

    year = start_date.year-1
    years = []
    while year <= end_date.year+1:
        years.append(year) 
        year +=1

    index_of_first_transition_before_start_date = 0
    index = 0
    transition_times = []
    for year in years:
        marchCal = calendar.monthcalendar(year, 3)
        first_week  = marchCal[0]
        second_week = marchCal[1]
        third_week  = marchCal[2]

        if first_week[calendar.SUNDAY]:
            dst_begins = second_week[calendar.SUNDAY]
        else:
            dst_begins = third_week[calendar.SUNDAY]

        dst_start = dt.datetime(year, 3, dst_begins, 2, tzinfo= tz)
        transition_times.append(dst_start)
        if dst_start < start_date:
            index_of_first_transition_before_start_date = index
        
        if end_date < dst_start:
            break
        
        novemberCal = calendar.monthcalendar(year, 11)
        first_week  = novemberCal[0]
        second_week = novemberCal[1]

        if first_week[calendar.SUNDAY]:
            dst_ends = first_week[calendar.SUNDAY]
        else:
            dst_ends = second_week[calendar.SUNDAY]
        
        dst_end = dt.datetime(year, 11, dst_ends, 2, tzinfo= tz)
        transition_times.append(dst_end)
        if end_date < dst_end:
            break
        
        index +=1
    
    return transition_times[index_of_first_transition_before_start_date:]

pjkaufman avatar Oct 25 '21 00:10 pjkaufman

as the vtimezone stuff seems daunting i started a conversation on twitter my understanding is that one should get the vtimezone from the underlying data

i may be able to create a POC of that by the weekend

RonnyPfannschmidt avatar Oct 28 '21 14:10 RonnyPfannschmidt

Greetings, as of my understanding the warnings are from pytz newest version. Is it worth it to make a smaller PR to fix the pytz version in the setup.py to a version that is working so the build is not broken(I think this warning introduced other unexpected behaviours - e.g. fresh clone and build - trying to create events - cannot change the hours range). For me it is not clear the state of this PR if there is anything I can help out with lmk I wish to contribute and accelerate this PR.

bancsorin10 avatar Oct 29 '21 10:10 bancsorin10

That may work, but the change happened as of python 3.9, so I think we would have to discontinue the use of python 3.9 in order for that to work, but I am aomewhat new to python. I still have some changes I have not committed because they are not ready yet, but I can see about adding to it this weekend

On Fri, Oct 29, 2021, 6:46 AM Turt7le @.***> wrote:

Greetings, as of my understanding the warnings are from pytz newest version. Is it worth it to make a smaller PR to fix the pytz version in the setup.py to a version that is working so the build is not broken(I think this warning introduced other unexpected behaviours - e.g. fresh clone and build - trying to create events - cannot change the hours range). For me it is not clear the state of this PR if there is anything I can help out with lmk I wish to contribute and accelerate this PR.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pimutils/khal/pull/1093#issuecomment-954643459, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHEDPO4MPST4CMISDL7NHSLUJKCSTANCNFSM5GQNTTHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

pjkaufman avatar Oct 29 '21 14:10 pjkaufman

@bancsorin10 Agreed. A separate PR pinning a working version of pytz is welcome, at least until we have a definitive version of PEP495 support.

WhyNotHugo avatar Nov 02 '21 21:11 WhyNotHugo

I made some changes to remove a lot of the pytz method calls and it seems to work for some things. The unit tests are failing (41 of them to be exact) and some of that is because I have not implemented logic for DST dates prior to 2007, I have no logic in place for BST dates, and the TZID is being listed at the timezone name instead of the ZoneInfo key.

However, I tried to commit my changes, but I got several errors from the precommit checks. They currently are preventing me from actually commiting to my own repo, so I will just list out some of the errors and ask for ideas on how to fix them.

The first issue it is really complaining about is the following:

try:
    from zoneinfo import ZoneInfo
except ImportError:
    from backports import zoneinfo as ZoneInfo

The precheck says the following about it:

khal/utils.py:29: error: Incompatible import of "ZoneInfo" (imported name has type Module, local name has type "Type[ZoneInfo]")
tests/utils.py:7: error: Incompatible import of "ZoneInfo" (imported name has type Module, local name has type "Type[ZoneInfo]")

Any ideas on how to fix this?

pjkaufman avatar Nov 03 '21 02:11 pjkaufman

I figured out how to get past the the precommit check failures for flake8, but I am not sure what I need to do to fix the actual errors. Please let me know what I should do to fix them. I am also unaware of what we should do to correctly replace transition time checking and BST time checking. Any ideas are welcome.

pjkaufman avatar Nov 03 '21 02:11 pjkaufman

@bancsorin10 Agreed. A separate PR pinning a working version of pytz is welcome, at least until we have a definitive version of PEP495 support.

from my testing I could not pinpoint the problem. I used python 3.9 with pytz version as old as 2013, and python 3.7.7 with the newest versions of pytz. It might be a combination of both or my system may just be too corrupted even tho I made the builds with a virtual python environment. I will try a few more versions and if I find something before this pr goes live I'll let you know.

bancsorin10 avatar Nov 03 '21 09:11 bancsorin10

@bancsorin10 The issue is likely with tzlocal rather than pytz directly, see my PR linked above.

Terrance avatar Nov 03 '21 09:11 Terrance

I have been able to determine that the TZID issue is probably an icalendar issue where it is not handling ZoneInfo and date util timezone names correctly: https://github.com/collective/icalendar/issues/291

I have suggested a change and can make a PR and see what happens, but I am not sure what to do if the issue is in icalendar and they do not choose to fix it.

pjkaufman avatar Nov 03 '21 11:11 pjkaufman

I can definitely continue working on these changes, but I will probably need some help with fixing the unit tests as I am not the most familiar with timezone information especially when stored using ical.

pjkaufman avatar Nov 22 '21 03:11 pjkaufman

Close-opened so CI will run, since GitHub seems to have dropped the last logs.

WhyNotHugo avatar Feb 03 '22 13:02 WhyNotHugo

Having a look at this now.

It's a bit unfortunate, that the only way to localize an existing datetime seems to be to use .replace(tzinfo=newtz), as this doesn't except like pytz's localize() did, if the existing datetime is already timezone aware.

geier avatar May 03 '22 16:05 geier

We might want to remove pytz from icalendar first, so we can get rid of all the ugly ZoneInfo(str(timezone))s.

geier avatar May 03 '22 16:05 geier

Thank you, @pjkaufman! This really helps!

I rebased this PR and fixed the first outstanding issue to #1125 for now.

Especially the timezone create_timezone() is a really ugly piece of code I never had the chance to properly clean up. I'll try to see if I can find the time to fix this outstanding issues (no promises).

geier avatar May 03 '22 17:05 geier

Regarding creating the VTIMEZONEs:

we can probably create something similar as before from ZoneInfo._ttinfos, Zoneinfo._trans_utc and Zoneinfo._fixed_offsets. But we could use the chance to read the timezone information values and extract the rrules from those (would lead to smaller VTIMEZONEs).

geier avatar May 03 '22 18:05 geier

Regarding creating the VTIMEZONEs:

we can probably create something similar as before from ZoneInfo._ttinfos, Zoneinfo._trans_utc and Zoneinfo._fixed_offsets. But we could use the chance to read the timezone information values and extract the rrules from those (would lead to smaller VTIMEZONEs).

That is quite interesting. I was unaware that this was a thing. I can see about adding it if you would like.

pjkaufman avatar May 03 '22 18:05 pjkaufman

We might want to remove pytz from icalendar first, so we can get rid of all the ugly ZoneInfo(str(timezone))s.

This would be great if it could be updated since it looks like pytz is being removed/deprecated.

pjkaufman avatar May 03 '22 18:05 pjkaufman

Regarding creating the VTIMEZONEs: we can probably create something similar as before from ZoneInfo._ttinfos, Zoneinfo._trans_utc and Zoneinfo._fixed_offsets. But we could use the chance to read the timezone information values and extract the rrules from those (would lead to smaller VTIMEZONEs).

That is quite interesting. I was unaware that this was a thing. I can see about adding it if you would like.

Yes, please go ahead.

geier avatar May 04 '22 11:05 geier

@pjkaufman you can also pull the rebased branch into this.

geier avatar May 04 '22 11:05 geier

Also the ics project seems to be able to create VTIMEZONEs, perhaps it's valuable to have a look at their approach: https://github.com/ics-py/ics-py/blob/main/src/ics/timezone/init.py

geier avatar May 04 '22 11:05 geier

Regarding creating the VTIMEZONEs: we can probably create something similar as before from ZoneInfo._ttinfos, Zoneinfo._trans_utc and Zoneinfo._fixed_offsets. But we could use the chance to read the timezone information values and extract the rrules from those (would lead to smaller VTIMEZONEs).

That is quite interesting. I was unaware that this was a thing. I can see about adding it if you would like.

Yes, please go ahead.

Could you please link out to the documentation for ZoneInfo._ttinfos, Zoneinfo._trans_utc and Zoneinfo._fixed_offsets as I cannot seem to find it? Without the documentation and the fact that python 3.9.7 keeps saying that

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.9/zoneinfo/__init__.py", line 27, in __getattr__
    raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
AttributeError: module 'zoneinfo' has no attribute '_ttinfos'

It may be that I am just misunderstanding what you mean here size it it unclear whether it is the zone info instance or the module that has the data on it. Thanks for the help!

pjkaufman avatar May 08 '22 22:05 pjkaufman