Skip to content
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

Feature/warn when using pytz #326

Closed

Conversation

svaningelgem
Copy link

As detailed in #325 : I would do something like this here.
I've added a print-out of the webpage in a docs/ folder. Just in case that it disappears from the internet (have had too many references disappear that way).

One thing: I couldn't run the tests on my windows machine because in conftest you have time.tzset(). I couldn't disable this without bringing the coverage to 99%; thus failing the workflow.

You don't encounter this issue because the workflow only tests on ubuntu. But it was a bit annoying 🤪.

My suggestion would be to remove conftest from the coverage report (as there's nothing in there anyway), and do something like I did in commit ddea169:

if hasattr(time, 'tzset'):  # Not available on Windows
    time.tzset()

Another way i tried was:

if sys.platform != "win32":
    time.tzset()

Anyway, minor annoyance only. Though would be nice to run the tests locally.
One other thing: only your main branch builds tests. There is a case to be made to let any branch test. WDYT?

@svaningelgem
Copy link
Author

@adamchainz , how to fix the precommit hook exactly?

@adamchainz
Copy link
Owner

I've added a print-out of the webpage in a docs/ folder. Just in case that it disappears from the internet (have had too many references disappear that way).

Please remove and instead ensure Paul’s site is added to the way back machine: https://archive.org/ . If it goes offline, we'll use that.

But don't worry, I'm sure Paul's site will be online for a while. I have a broken link checker running on my blog, and several times I've spotted Paul's certificate expiring and pinged him to fix it :)

One thing: I couldn't run the tests on my windows machine because in conftest you have time.tzset(). I couldn't disable this without bringing the coverage to 99%; thus failing the workflow.

You don't encounter this issue because the workflow only tests on ubuntu. But it was a bit annoying 🤪.

Happy to try with your suggestion if it makes the tests work on Windows locally. I don't think running tests on Windows on CI is worth the extra wait times right now, if it's just that tzset that's the problem.

One other thing: only your main branch builds tests. There is a case to be made to let any branch test. WDYT?

Tests run when you open a PR. If you want to run the tests on CI, open a PR. You can make it a draft if you don't want to ping me.

If you set tests to run on any branch, they run twice on each commit to a PR, wasting time/energy.

Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs a changelog note

requirements/compile.py Outdated Show resolved Hide resolved
except ImportError:
import subprocess

subprocess.run(["pip", "install", "pytz"])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't run pip during tests, we can instead add pytz to requirements.in and recompile the test requirements

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell me how to recompile it? As I only have Windows available, I'm afraid when I run the script it'll mess up the hashes and stuff...

src/time_machine/__init__.py Outdated Show resolved Hide resolved
src/time_machine/__init__.py Outdated Show resolved Hide resolved
@svaningelgem
Copy link
Author

Please remove and instead ensure Paul’s site is added to the way back machine: https://archive.org/ . If it goes offline, we'll use that.

A snapshot was captured. Visit page: /web/20230215195634/https://blog.ganssle.io/articles/2018/03/pytz-fastest-footgun.html

@svaningelgem
Copy link
Author

I've another question:
What about this:

        elif HAVE_PYTZ and isinstance(dest.tzinfo, pytz.BaseTzInfo):
            if sys.version_info[:2] >= (3, 9):
                dest = dest.replace(tzinfo=zoneinfo.ZoneInfo(dest.tzinfo.zone))
            else:
                raise TypeError(
                    "We don't support pytz. For more background information, please "
                    "read the pytz topic in the readme."
                )

I'm not quite certain it's the right way to do. Because the datetime might be already eagerly calculated, and then changing the timezone information will mess it up... So maybe erroring out completely is the safer way.
Just thought I'd propose it here :)

@svaningelgem svaningelgem closed this by deleting the head repository Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants