Skip to content

SetManager inherits an API surface it doesn't want; decide its real interface #243

Description

@derek73

Summary

SetManager subclasses collections.abc.Set, and most of its sharp edges trace back to that choice. 1.3.0 made the inherited surface safe (#238, PR #240); 2.0 should decide whether it should exist.

Status after the 1.3.0 release: the parts common to both options already shipped as deprecations — __call__ and missing-member remove() emit DeprecationWarning, and discard() exists as the ignore-missing replacement (#253). The remaining decision here is only the option-A-vs-B question (the operator surface and the Set ABC), plus executing the removals.

Symptoms, all one root cause

  1. Wrong ABC. collections.abc.Set is the immutable set interface, yet the class exposes add()/remove()/clear(). The honest ABC for the current behavior is MutableSet — but see below for whether we want the ABC at all.
  2. remove() has discard() semantics. It silently ignores missing members, the opposite of what set.remove promises. CONSTANTS.titles.remove('typo-entry') reports nothing. (1.3.0: now emits DeprecationWarning; discard() added.)
  3. __call__ is an escape hatch. CONSTANTS.titles() returns the raw underlying set, allowing mutation that bypasses both normalization and the _on_change cache-invalidation hook that Constants relies on. (1.3.0: now emits DeprecationWarning.)
  4. The operator surface exists only by inheritance. The Set mixins inject |/&/-/^ operators that accept arbitrary iterables and construct results outside the class's normalization invariant. Making them safe took six guarded, operand-normalizing overrides with ~10 type: ignore comments (PR Fix bare string to SetManager silently shredding into characters #240) — when you fight both typeshed and the runtime ABC to guard methods you never designed, the supertype is wrong.

Related: the __contains__ normalization asymmetry is now tracked separately as #244 (1.3.0); whichever option lands here, membership survives unchanged.

Options

A. Stop subclassing Set; compose over a plain set (recommended).
Expose exactly what the config use case needs: add, remove, discard, clear, in, iteration, len. The one internal set-algebra use, the suffixes_prefixes_titles union, works on the underlying sets directly.

Pros:

  • Smallest honest surface — every method exists on purpose, and all mutation is forced through add()/remove(), so normalization and the _on_change cache-invalidation hook can never be bypassed
  • Deletes the six operator overrides and their ~10 type: ignore comments outright; nothing inherited is left to audit when Python's ABC mixins evolve
  • No future symptom 5: the class can't grow new behavior without someone deliberately adding it

Cons:

  • isinstance(x, collections.abc.Set) stops passing; any downstream duck-typed set algebra on config sets breaks
  • |/&/-/^ on config sets stop working (mitigation: the operators only became safe in 1.3.0, so correct usage in the wild is necessarily rare)
  • Loses the ABC's free set-comparison semantics (==, <= between managers); a deliberate two-line __eq__ can be added back if wanted

B. Commit to the surface properly: switch to MutableSet.
Keep the normalized operators from PR #240, give remove() real KeyError semantics (discard() already shipped in 1.3.0), drop __call__ (already deprecated).

Pros:

  • Familiar set idioms keep working, including isinstance checks
  • MutableSet's in-place mixins are a real fit: __ior__ routes each element through add(), so titles |= [...] would normalize and fire _on_change for free — the inherited behavior finally aligns with the class invariant
  • Least migration for anyone using operators today

Cons:

  • Keeps the root cause: a large inherited surface where every current and future mixin must be audited against the normalization invariant — MutableSet adds more, not less (pop, __iand__, __ixor__, __isub__, each of which mutates and must be verified to fire _on_change)
  • The typeshed-vs-runtime annotation friction from PR Fix bare string to SetManager silently shredding into characters #240 remains and grows
  • Same breaking changes to remove()/__call__ anyway, so B pays most of A's migration cost while keeping the maintenance tax

Option A is preferred: no parser functionality needs set algebra on config sets, and every symptom above came from surface nobody designed. B's strongest point — __ior__ normalizing for free — fixes one operator by accident of mixin implementation, which is the same coupling that produced the bugs.

Breaking changes (hence 2.0)

  • isinstance(x, collections.abc.Set) checks stop passing (option A)
  • |/&/-/^ on config sets stop working (option A) or keep 1.3.0 behavior (option B)
  • remove() of a missing member raises (discard(), shipped in 1.3.0, is the ignore-missing spelling)
  • CONSTANTS.titles() call syntax removed — iterate or copy instead

Metadata

Metadata

Assignees

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions