-
Notifications
You must be signed in to change notification settings - Fork 207
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
Update khal to Follow PEP495 Removing Deprecation Warnings #1093
base: master
Are you sure you want to change the base?
Conversation
Correct me if I am wrong, but it looks like Thank you for letting me know! |
Note: I did miss the |
This was taken care of earlier. I am now just waiting on guidance regarding the pre-commit issue. |
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. |
@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:
Also BST would need a similar solution. What do you think about this? |
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.
|
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 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? |
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:
Here is the DST algorithm (this is more a proof of concept than an actual final algorithm):
|
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 |
Greetings, as of my understanding the warnings are from |
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
<#1093 (comment)>, 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>.
|
@bancsorin10 Agreed. A separate PR pinning a working version of pytz is welcome, at least until we have a definitive version of PEP495 support. |
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:
The precheck says the following about it:
Any ideas on how to fix this? |
I figured out how to get past the the precommit check failures for |
khal/icalendar.py
Outdated
try: | ||
from zoneinfo import ZoneInfo | ||
except ImportError: | ||
from backports import zoneinfo as ZoneInfo |
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.
For some reason flake8 does not like this. Any ideas on how to fix it?
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.
is this problem still up? Is there a full log of the error. I will look into it and come back
khal/khalendar/event.py
Outdated
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 = ( | ||
"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:] |
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.
Here is an idea for how to get transition dates without pytz.
khal/khalendar/event.py
Outdated
tz.fromutc(tz._utc_transition_times[num]).replace(tzinfo=None)) | ||
subcomp.add('TZOFFSETTO', tz._transition_info[num][0]) | ||
subcomp.add('TZOFFSETFROM', tz._transition_info[num - 1][0]) | ||
tz.fromutc(transition_times[index]).replace(tzinfo=None) |
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.
For some reason the TZID
is being listed as the timezone name (i.e. EDT, CET, etc.) instead of the passed in name for the ZoneInfo (i.e. "UTC", "America/New_York", etc.).
khal/settings/utils.py
Outdated
try: | ||
import zoneinfo as ZoneInfo | ||
except ImportError: # I am not sure if this is correct for the backport | ||
from backports import zoneinfo as ZoneInfo |
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 am not sure if this is correct for the backport as I did not change it while I imported the module instead of a module inside zoneinfo for the try portion of the try except.
khal/controllers.py
Outdated
try: | ||
import zoneinfo as ZoneInfo | ||
except ImportError: # I am not sure if this is correct for the backport | ||
from backports import zoneinfo as ZoneInfo |
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.
from backports import zoneinfo as ZoneInfo | |
from backports.zoneinfo import ZoneInfo |
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 did what it says to do in the documentation I could find: https://pypi.org/project/backports.zoneinfo/
For some reason
from backports.zoneinfo import ZoneInfo
Yields the following warning: Import "backports.zoneinfo" could not be resolved
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.
When flake8 runs on Python3.9, this library won't be installed. It's safe to ignore this specific error.
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 The issue is likely with tzlocal rather than pytz directly, see my PR linked above. |
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: collective/icalendar#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. |
…d BDT and DST value determination for all reasonable dates
Could you please link out to the documentation for
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! |
I don't think there is any documentation for those _ variables apart from the source code |
But I now see that there is also a C implementation of the module (which probably gets loaded for you). I don't think we can access those transition times if the C implementation gets used, but I'd need to have a look into this. |
It looks like if we import ZoneInfo like this
we can force to import the python version. Also, we would need to first upgrade collectivie/icalendar to use tzinfo to completely switch to tzinfo. But the current vtimezone implementation definitively is a mess and needs an upgrade. Perhaps it's more valuable, to have a look at how the ZoneInfo implementation loads the timezone, but I'm not sure what the best way forward here is at the moment. |
I plan to make a couple of changes yet on the master branch of my fork, but I would like to ask that someone that uses this app takeover the changes from there since I no longer use this app which means that I don't have a good way to verify that it is working correctly beyond the UTs which may not be extensive enough to be considered proof that the app is working, besides the fact that my changes modify them as well. I would also recommend updating icalendar to allow for use without pytz as that was a major blocker that has drawn out the updating of this repo. Beyond that, I plan to see about swapping out the logic for timezone info and finishing the rebase of the branch before stopping making changes to my fork. I do hope that this change gets implemented as it seems necessary. I also hope that the code that has been added here will aid in the transition from pytz. Please let me know if there would be a problem with me finishing the rebase and making the timezone transition times change before closing out the PR to allow another dev to hopefully take over. |
… which removed warnings and added myself to the authors list as per the contributing instructions
for more information, see https://pre-commit.ci
…d BDT and DST value determination for all reasonable dates
for more information, see https://pre-commit.ci
…c which supposedly will work based on comments on the PR
for more information, see https://pre-commit.ci
…ome of the pre commit complaints
…ome of the pre commit complaints
for more information, see https://pre-commit.ci
@WhyNotHugo , @geier , I have made several of the changes requested, but I am not able to build it locally and do not have the expertise to figure out the python errors. However since I plan to relinquish the remaining work and allow someone that is familiar with python and is using khal to complete this PR, I do not see resolving the local build problems due to bad dependency installations as an issue. Hopefully this will PR can serve as a base for making the change over to no longer use pytz. I hope that the dependencies get updated soon to be compliant with no longer using pytz to help this project keep moving forward. Thank you for letting me try my hand at creating this fix. |
When running
khal ...
orikhal ...
an issue would arise where warnings about deprecated functions being used (see #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:
khalendar.py
andevent.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.