feat(jmap)!: re-export caldav.jmap from the standalone calendaring-jmap package - #711
SashankBhamidi wants to merge 8 commits into
Conversation
|
This is on my list, and I expect to have it merged to main tomorrow. Before approving this pull request, I should ensure all tests passes. I just released version 3.3.1, so at least the conflict in CHANGELOG needs some massaging. Background: The JMAP logic was requested in the CalDAV-library based on an old user request, but it doesn't really belong in the CalDAV-library. Sashank has now been reimplementing the logic in a separate JMAP-library, and the code should be yanked out of the CalDAV-libary. To try not to break backward-compatibility, the CalDAV-library will come with an optional dependency on the JMAP-library and wrapper functions. If it's hard to maintain backward-compatibility and one has a good reason for it, then breaking changes are OK - because the JMAP-support in CalDAV 3.x was marked up as experimental in the documentation. |
|
With jmap as an optional dependency, the import will break with a bare |
bc7e2fe to
70162c5
Compare
|
Wrapped the calendaring_jmap import in try/except with a descriptive error pointing at |
The re-export shim carried only the top-level names, so the import line the 3.3 documentation spells out verbatim - `from caldav.jmap.error import JMAPAuthError` - died with a ModuleNotFoundError instead of going through the new DeprecationWarning. calendaring-jmap mirrors the module layout caldav.jmap used to have, so each public submodule is now aliased into sys.modules and bound as an attribute of the package. Prompt: look into PR [#711] Assisted-By: Claude Opus 5 <noreply@anthropic.com>
The CHANGELOG said old imports keep working, but not that a plain `pip install caldav` no longer gives you caldav.jmap at all, and neither it nor jmap.rst mentioned that calendaring-jmap is AGPL-3.0-or-later where caldav is GPL-3.0-or-later OR Apache-2.0. Same import path, different terms - easy to miss for anyone who picked the Apache option. Both are now written up as breaking changes, and the JMAP page carries a licensing note next to the install instructions. Prompt: look into PR [#711] Assisted-By: Claude Opus 5 <noreply@anthropic.com>
tests/test_jmap_wrapper.py imported calendaring_jmap at module level, so the documented developer setup - `pip install -e ".[test]"`, which does not carry the jmap extra - ended in a collection error rather than a skip. Only tox installs the extra, so CI never saw it. Resolved with pytest.importorskip, the convention the rest of the test suite already uses for an optional dependency, rather than by adding calendaring-jmap to the test extra: that would put an AGPL package into every contributor's test environment. Prompt: look into PR [#711] Assisted-By: Claude Opus 5 <noreply@anthropic.com>
http-libraries.rst still described the async JMAP client as the one exception to the fallback chain, "built on niquests' AsyncSession" - that is require_async_session() in caldav/lib/http_sync.py, which the JMAP extraction deleted. The choice of HTTP library is calendaring-jmap's own concern now, and the advice to depend on caldav[niquests] for it was moot anyway, since calendaring-jmap requires niquests outright. Prompt: look into PR [#711] Assisted-By: Claude Opus 5 <noreply@anthropic.com>
required_library_error()'s only caller was require_async_session(), which went with the JMAP extraction, and its tests pinned the exact wording using "the async JMAP client" as the example - a string naming something no longer in the tree, in tests that can no longer go red for a real user. The helper is kept, since the condition it describes recurs whenever a component is built on a single library, but the example is now a hypothetical. The AsyncSession and require_async_session removals from caldav.lib.http_sync get the CHANGELOG line they were missing. Prompt: look into PR [#711] Assisted-By: Claude Opus 5 <noreply@anthropic.com>
|
I asked Claude to have a look, and it did record some "breaking changes":
The first one is already an accepted compromise, the license thing I've also explicitly accepted, but the middle one seems a bit silly. Is there anything in the current jmap code that needs icalendar 7.3.0? If not, could you release one version with stated support for icalendar 6? It also seems silly to have both requests and niquests as hard dependencies in the project. (see https://github.com/python-caldav/caldav/blob/master/docs/source/http-libraries.rst for some information on requests vs niquests) |
|
Rather than bouncing a ball, I decided to ask claude to amend the pull request. It added some few commits - and apparently broke the tests while being at it. I've looked a bit through, some of the changes it did seems sound. I'm not sure I'm sharing the concern that it's a crime to let test code depend on an AGPL-library. At least things should be squashed before merging to the main branch. |
|
@tobixen 7.3.0 isn't arbitrary. GHSA-qjcq-q7h7-r74v, the The other reasonn is the globally-unique-TZID resolution (issue #313), the 6.x is off the table. Because it's missing both of those, and I'm not going to loosen a floor I set for a reason just to make the pin number smaller. Checked the changelog against what's coming up on my roadmap too, nothing past 7.3.0 is needed, so this holds for a while. The I will let you know when I make the next release with this fix. |
|
That fix is merged now, main is at 514b84c, if you want to pin it before I cut a release: calendaring-jmap @ git+https://github.com/pycalendar/calendaring-jmap.git@514b84c Once I tag a release you can swap that for a normal version pin. |
Summary
caldav/jmap/no longer carries its own copy of the JMAP client; it now re-exports the standalonecalendaring-jmappackage (v1.1.0), added as an optional dependency (caldav[jmap]).from caldav.jmap import JMAPClientstill works, but now emits aDeprecationWarningpointing at the new canonicalfrom calendaring_jmap import JMAPClient.get_jmap_client()/get_async_jmap_client()keep resolving configuration throughcaldav.config.get_connection_params()exactly as before.DAVError, via a conditional import on calendaring-jmap's side (feat: conditionally subclass caldav's DAVError when caldav is present pycalendar/calendaring-jmap#43, released as 1.1.0).Closes pycalendar/calendaring-jmap#10