Preserve explicit VALUE parameter on jCal round-trip (fixes #1426)

[gaoflow]

Summary

A round-trip through jCal (to_jcal() → from_jcal()) silently drops an explicit VALUE parameter. For example RDATE;VALUE=PERIOD:… and TRIGGER;VALUE=DATE-TIME:… come back without their VALUE parameter. This is #1426, which @stevepiercy confirmed.

Cause

jCal stores a property's value type in the type field (the third array item, e.g. "period"), not as a VALUE parameter. Component.from_jcal only handled the redundant case — it deleted VALUE when the type matched the default — but it never restored a VALUE parameter for a non-default type:

if prop_cls == cls.types_factory.for_property(prop_name):
    del v_prop.VALUE

That class comparison can't distinguish the cases anyway: vDDDTypes represents DATE, DATE-TIME and DURATION, and RDATE/EXDATE always resolve to vDDDLists, so for_property returns the same class regardless of the type field. As a result the VALUE parameter was always dropped.

Fix

Restore the VALUE parameter whenever the jCal type differs from the property's default value type, and otherwise leave it off:

default_type = cls.types_factory.default_value_type(prop_name)
if isinstance(prop_value, str) and prop_value.lower() != default_type:
    v_prop.VALUE = prop_value.upper()
elif "VALUE" in v_prop.params:
    del v_prop.VALUE

The new TypesFactory.default_value_type(name) returns the default jCal value type for a property. It uses types_map, but for internal type keys that aren't jCal value types themselves (e.g. CATEGORIES → text, RDATE/EXDATE → date-time) it asks the value class for the type it actually serialises to, so default-typed properties never gain a spurious VALUE parameter.

Behaviour

property	before	after
RDATE;VALUE=PERIOD	{}	{'VALUE': 'PERIOD'}
TRIGGER;VALUE=DATE-TIME	{}	{'VALUE': 'DATE-TIME'}
DTEND;VALUE=DATE	{}	{'VALUE': 'DATE'}
DTSTART (default)	{}	{}
CATEGORIES (default)	{}	{}

Tests

Added test_jcal_round_trip_preserves_value_parameter, covering both explicit non-default types (preserved) and default types including CATEGORIES (no spurious parameter). It fails on main for the non-default cases and passes with this change. Full suite: 9601 passed, 0 failed; ruff clean. Added a news/1426.bugfix towncrier fragment.

Fixes #1426

---

📚 Documentation preview 📚: https://icalendar--1434.org.readthedocs.build/en/1434/

[read-the-docs-community]

Documentation build overview

📚 icalendar | 🛠️ Build #32958317 | 📁 Comparing 56a78ae against latest (b0bbea4)

  🔍 Preview build  

6 files changed · ± 6 modified

± Modified

• 404.html
• genindex.html
• reference/api/icalendar.prop.factory.html
• reference/api/icalendar.prop.html
• _modules/icalendar/cal/component.html
• _modules/icalendar/prop/factory.html

[lcampanella98]

Thanks for picking this up, @gaoflow — solid fix. Heads up: I'd been working on #1426 in parallel (@stevepiercy had asked me to look into it), and landed on the same approach — a default_value_type() helper + restoring VALUE from the jCal type field. So rather than open a competing PR, let me just offer what my branch adds.

Tests are the main difference. I ran my suite against your fix and it all passes, so they're purely additive:

• a VALUE-conformance check wired into the existing jCal round-trip test (runs across all calendar fixtures × pytz/zoneinfo);
• an independent, RFC-derived reference table for default_value_type() (~58 properties), catching a regression in that lookup independently of from_jcal;
• a fixture-based regression test that also runs under LazyCalendar.

One edge case I noticed: for a registered property with jCal type unknown, #1434 emits VALUE=UNKNOWN, which §5.2 forbids (UNKNOWN is reserved from iCalendar):

Component.from_jcal(["vevent", [["dtstart", {}, "unknown", "x"]], []]).to_ical()
# #1434:    DTSTART;VALUE=UNKNOWN:x
# expected: DTSTART:x

Niche, one-line guard; I have a test for it (test_issue_1426.py::test_unknown_jcal_type_does_not_add_value_parameter).

Happy to fold the tests + guard into #1434, or pull from my branch: https://github.com/lcampanella98/icalendar/tree/fix-1426-value-param. Whatever's least work for you all — thanks for moving this forward!

[gaoflow]

Thanks @lcampanella98 — and good catch on the unknown case. You're right that a registered property with jCal type unknown was getting a forbidden VALUE=UNKNOWN (RFC 7265 §5.2); my existing unknown test only went through vUnknown.from_jcal directly, so it never exercised the Component.from_jcal path where the default-type comparison adds the parameter.

I've folded the guard into this PR (just under the existing comparison — unknown is now treated like the default type, so no VALUE is reconstructed) and added your test_unknown_jcal_type_does_not_add_value_parameter through Component.from_jcal, with you credited as co-author on the commit. DTSTART;VALUE=UNKNOWN:x now round-trips back to DTSTART:x. Full jCal suite stays green (1201 passed).

I kept this PR's implementation since it's already under review, but your independent RFC-derived reference table for default_value_type() sounds like genuinely worthwhile extra coverage — happy to pull that in here too if you'd like, or it could go in as a follow-up. Whatever @stevepiercy prefers. Thanks again for flagging this rather than racing a second PR.

[stevepiercy]

@lcampanella98 @gaoflow thanks for your collaboration and coordination.

sounds like genuinely worthwhile extra coverage — happy to pull that in here too if you'd like, or it could go in as a follow-up.

@gaoflow would you please pull that in, as @lcampanella98 has granted permission to incorporate his work, and add their username to the news fragment to give you both credit? Thank you!