Warn on cached lookups of guessed globally unique TZIDs

[codeAnqiang-ma]

Linked issue

• Closes [Bug] Cached TZID guess swallows GloballyUniqueTZIDGuessed on repeated lookups; test suite fails without pytz (nopytz CI env installs pytz) #1660

Description

Guessed timezones were cached under the cleaned TZID, so only the first lookup of a globally unique TZID warned; later lookups hit the cache and stayed silent. The cache now remembers guesses and warns on hits too. It also fixes a test-order dependence in pytz-free runs (see the linked issue). A regression test fails before and passes after.

Checklist

• I added a change log entry, following the instructions in Change log entry format.
• I followed icalendar's Artificial intelligence policy and disclosed my Responsible AI use in my commit messages, if applicable.
• I added or updated tests, if applicable.
• I ran and ensured all tests pass locally by following Run tests.
• I added or edited documentation as necessary, both as docstrings to be rendered in the API documentation and narrative documentation, following the Style guide.

Additional information

AI disclosure: drafted with AI (Claude via Cursor); reproduced, reviewed, and verified by me.

[github-actions]

Profile summary:

GitHub user: codeAnqiang-ma
🟡 Some concerns found with user's profile.
🟢 No concerns found with recent PR activity.
🟡 Some concerns found with recent issue activity.

For a more detailed report, run `gh-profiler codeAnqiang-ma`.

Full profile

GitHub user: codeAnqiang-ma
🟡 Some concerns found with user's profile.
   🟡 Account age: 4 months
   🟡 Profile information:
        name: anchor
      Empty fields: company, blog, location, email, bio

🟢 No concerns found with recent PR activity.
   23 PRs opened in the last 21 days.
      0 opened against repos the user owns.
      0 opened against repos in publicly associated orgs.
      23 opened against external repos.

   🟢 0 of 23 external PRs closed without merging in the last 21 days.

🟡 Some concerns found with recent issue activity.
   34 new issues opened in the last 21 days.
      0 opened in repos the user owns.
      0 opened in repos in publicly associated orgs.
      34 opened in external repos.

   🟢 3 external issues closed as NOT_PLANNED.
   🟡 4 external issues opened with the same title:
        [BUG] Bare date literals in date-typed formula args are evaluated as arithmetic — get_year(2025-01-15) returns 2009 (2)
        /v1/completions returns 500 server_error for an empty prompt instead of the documented fail-closed 400 (2)

[github-actions]

This pull request did not pass quality checks and AI use is suspected. Please review Contribute and make any necessary amendments.

[read-the-docs-community]

Documentation build overview

📚 icalendar | 🛠️ Build #34037719 | 📁 Comparing 0213619 against latest (30ec6ee)

  🔍 Preview build  

3 files changed

± 404.html
± reference/api/icalendar.timezone.tzp.html
± _modules/icalendar/timezone/tzp.html

[codeAnqiang-ma]

Amendments per the quality-check feedback: I shortened the commit message and the PR description, keeping the AI disclosure in the commit message and the change log entry per the Responsible AI use policy. The fix and its regression test are unchanged.

The earlier Documentation and Read the Docs failures were transient network errors while provisioning the build environment; all checks pass after the re-run.

[niccokunzmann]

Thanks for the PR!

Is it normal practice to warn several times?

[niccokunzmann]

To be clear for you: I did neither understand your issue and also not your PR description. But the code makes sense!

Could you in future just write your own, small descriptions?

[codeAnqiang-ma]

Is it normal practice to warn several times?

Yes — the usual practice is to emit on every occurrence and let the user's warning filters decide what is shown, like DeprecationWarning. With default filters Python deduplicates the display anyway: parsing the same calendar three times still prints one message per distinct TZID (verified locally). Emitting every time just keeps pytest.warns and simplefilter("error")/"ignore" working independently of cache state. If you'd prefer a single warning per TZID, I'm happy to change that.

Could you in future just write your own, small descriptions?

Fair point — I'll keep future descriptions short.

[lcampanella98]

Nice PR.

One suggestion on the test below, and one question on scope.

This says Closes #1660, but the issue reports two problems and this fixes one. [testenv:nopytz] still inherits dependency_groups = test, which lists pytz>=2025.2, so the 3.10 (nopytz) job keeps running with pytz and would not catch a repeat of this. Fine with me to leave that fix out of this PR, but then better not to auto-close the issue in my opinion.

[lcampanella98]

I think it'd be good to pin the message as well which changed from using lookup_id to guessed_id which are different on cache hit

Suggested change

	tzid = "/mozilla.org/20070129_1/America/New_York"
	with pytest.warns(GloballyUniqueTZIDGuessed):
	first = tzp.timezone(tzid)
	with pytest.warns(GloballyUniqueTZIDGuessed):
	second = tzp.timezone(tzid)
	assert first is second
	tzid = "/mozilla.org/20070129_1/America/New_York"
	guess = "guessing it means 'America/New_York'"
	with pytest.warns(GloballyUniqueTZIDGuessed, match=guess):
	first = tzp.timezone(tzid)
	with pytest.warns(GloballyUniqueTZIDGuessed, match=guess):
	second = tzp.timezone(tzid)
	assert first is second

[Solaris-star]

Verified the current head 0213619845d5ff2525de409d70dd40c0fe1d5f1f locally.

• PYTHONPATH=src uv run --no-project --python 3.13 --with pytest --with python-dateutil --with tzdata --with typing-extensions --with pytz python -m pytest src/icalendar/tests/test_issue_313_globally_unique_tzid.py -q — 22 passed.
• The same command without --with pytz — 11 passed.
• git diff --check 0213619845d5ff2525de409d70dd40c0fe1d5f1f^ 0213619845d5ff2525de409d70dd40c0fe1d5f1f — clean.

The cache-warning fix and regression test behave correctly in both provider configurations. I did not run the full tox/docs matrix. The remaining merge blocker appears to be scope/issue linkage rather than this code path: #1660 also tracks the nopytz tox dependency problem, while this PR fixes the cache-warning behavior only. Changing Closes #1660 to a non-closing reference (and adjusting the news wording if needed) would align with the collaborator feedback.

[niccokunzmann]

@Solaris-star Thanks for your review and continuous engagement with the project!
In future, when people contribute, it can be overwhelming to read a technical comment like this. Doing this has worked for me:

• Thank for the PR - we are all volunteers here, so our time is a gift!
• Give new contributors a hand in understanding the project, assess where they are at. E.g. for your comment - I do not know what to do with it - are changes needed? Do you think the issue is addressed?
• Which type of comment helps get the PR merged, not just the code but what does the person need?
• Ask a question at the end so that contributors know how to engage with your review. E.g. broadly "What do you think?"

So, how is that for you?

[Solaris-star]

Thanks for the reminder — noted! Let me condense my conclusion into an actionable single point: the code at head 0213619845d5ff2525de409d70dd40c0fe1d5f1f does fix the warning behavior for cached globally-unique TZIDs. However, #1660 also contains a separate tox configuration problem where [testenv:nopytz] still inherits pytz. So I would prefer this PR not auto-close all of #1660; changing it to Part of #1660 (or Relates to #1660) would be more accurate, with the nopytz config handled separately.

I re-verified the current head myself:

• PYTHONPATH=src uv run --no-project --python 3.13 --with pytest --with python-dateutil --with tzdata --with typing-extensions --with pytz python -m pytest src/icalendar/tests/test_issue_313_globally_unique_tzid.py -q → 22 passed (12 warnings).
• The same command without --with pytz → 11 passed (6 warnings).
• git diff --check 0213619845d5ff2525de409d70dd40c0fe1d5f1f^ 0213619845d5ff2525de409d70dd40c0fe1d5f1f → clean.
• Tracked secret scan covered 708 tracked files with 0 hits.

I did not run the full tox suite, docs build, or the full-matrix CI, so those checks should still be confirmed against remote CI. Going forward I'll keep reviews to short conclusions, blockers, and concrete next steps to avoid extra reading burden for contributors.

[niccokunzmann]

@Solaris-star 谢谢您的回复！能否请您将其翻译成英语，并修改一下您的评论？

Thank you for the response! Could you translate it to English and edit your comment, please?

[SemTiOne]

This is a good PR. I don't see any bugs. Issue #1660 is real also.

I have 1 question below.

[niccokunzmann]

@codeAnqiang-ma your PR receives a lot of attention. Let us know how you would like to proceed. If you have any questions or if we can clarify sometjing, please let us know!

[stevepiercy]

@codeAnqiang-ma would you please take a look at the feedback and respond in kind? This is nice work. Thank you!

[stevepiercy]

Minor spelling tweak.

Suggested change

Emit :class:`~icalendar.error.GloballyUniqueTZIDGuessed` on every lookup of a globally unique TZID (:rfc:`5545#section-3.2.19`), including lookups served from the TZID cache. Previously only the first lookup on a ``TZP`` instance warned: the guessed timezone was cached under the cleaned id, so repeated lookups returned silently, which also made ``test_timezone_id_resolves`` order-dependent and fail in pytz-free environments. Drafted with AI assistance; reproduced, reviewed, and verified by the author. @codeAnqiang-ma

Emit :class:`~icalendar.error.GloballyUniqueTZIDGuessed` on every lookup of a globally unique TZID (:rfc:`5545#section-3.2.19`), including lookups served from the TZID cache. Previously only the first lookup on a ``TZP`` instance warned: the guessed timezone was cached under the cleaned ID, so repeated lookups returned silently, which also made ``test_timezone_id_resolves`` order-dependent and fail in pytz-free environments. Drafted with AI assistance; reproduced, reviewed, and verified by the author. @codeAnqiang-ma