Skip to content

Commit f7e2a35

Browse files
committed
docs: Explain the frequenz.core.warnings helpers in the wrapping guide
The previous commits moved the library and its tests to the `frequenz.core.warnings` helpers, but the guide still told contributors to silence inner deprecations with `warnings.catch_warnings()`, which is the exact bug they fix. Anyone following it would bring the warning amplification back. The deprecation guide gets two new sections and an updated one: * Keeping a moved symbol importable with `deprecated_aliases()`, including the `TYPE_CHECKING`/`else` structure it needs, a message carrying the version, and when a deprecated class is the better fit, as with `ComponentId`. * Silencing only the deprecations the library raises itself, with `ignoring_deprecations()` around the one statement, and why `catch_warnings()` must not be used for it. * Testing that a replacement doesn't warn with `asserting_no_deprecations()` instead of an "error" filter. The testing guide points at both helpers. The helpers are mentioned as plain code rather than cross-references because the frequenz-core inventory will only have them once v1.5.0 is released. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
1 parent 4bd97e0 commit f7e2a35

3 files changed

Lines changed: 103 additions & 10 deletions

File tree

‎docs/wrapping-guide/deprecation-and-compatibility.md‎

Lines changed: 94 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,80 @@ class Event(Enum):
120120
"""Event when a new resource is created."""
121121
```
122122

123+
## Keep a moved symbol importable
124+
125+
When a public symbol moves to another module, keep its old import path working
126+
with `frequenz.core.warnings.deprecated_aliases()` instead of writing a module
127+
`__getattr__` by hand. The alias is the very same object, so
128+
[`isinstance()`][isinstance] keeps working through both paths, and the
129+
documentation build generates the admonition and label for every alias in the
130+
table, as long as the table and the message are literals in the call.
131+
132+
```python
133+
from typing import TYPE_CHECKING, TypeAlias
134+
135+
from frequenz.core.warnings import deprecated_aliases
136+
137+
if TYPE_CHECKING:
138+
from example.new import Thing as _Thing
139+
140+
Thing: TypeAlias = _Thing
141+
"""A thing, now living in `example.new`."""
142+
else:
143+
__getattr__ = deprecated_aliases(
144+
__name__,
145+
{"Thing": "example.new"},
146+
message="{old} is deprecated since v0.5.0. Use {new} instead.",
147+
)
148+
```
149+
150+
Keep that structure exactly: without the `else:`, type checkers see the
151+
`__getattr__` and treat every name in the module as `Any`. Pass `message` so
152+
the warning carries the version like every other deprecation here. `{old}` and
153+
`{new}` become the two fully qualified names, and the documentation turns
154+
`{new}` into a link, so leave out the cross-reference brackets.
155+
156+
An alias only fits when the old name can be the same object as the new one.
157+
When the old type has to stay distinct, as
158+
[`ComponentId`][frequenz.client.common.microgrid.components.ComponentId] does
159+
next to
160+
[`ElectricalComponentId`][frequenz.client.common.microgrid.electrical_components.ElectricalComponentId],
161+
keep a deprecated class instead.
162+
163+
## Silence only the deprecations you raise yourself
164+
165+
Sometimes library code has to touch a symbol it deprecated itself, such as a
166+
converter comparing a value against a deprecated `UNSPECIFIED` member. The
167+
caller did nothing deprecated, so the warning is noise. Silence it with
168+
`frequenz.core.warnings.ignoring_deprecations()`, around the statement that
169+
raises it and nothing more, so deprecations from anywhere else still get
170+
through:
171+
172+
```python
173+
from frequenz.core.warnings import ignoring_deprecations
174+
175+
from example import Event
176+
177+
178+
def event_to_str(event: Event | int) -> str:
179+
with ignoring_deprecations():
180+
unspecified = Event.UNSPECIFIED
181+
if event in (0, unspecified):
182+
return "<invalid:0>"
183+
return event.name if isinstance(event, Event) else f"<unknown:{event}>"
184+
```
185+
186+
The same applies to a deprecated function calling another deprecated symbol:
187+
its caller gets the one public warning of the outer function, and the inner one
188+
is silenced.
189+
190+
Do not use [`warnings.catch_warnings()`][warnings.catch_warnings] for this.
191+
Entering and leaving it resets the warnings deduplication history of the whole
192+
program ([python/cpython#73858](https://github.com/python/cpython/issues/73858)),
193+
so every warning that was already shown, by this library or any other code, is
194+
shown again after each call. In an application converting data in a loop, that
195+
turns a handful of warnings into tens of thousands.
196+
123197
## Tighten invariants in stages
124198

125199
When you tighten a rule, do not always reject old input immediately. First,
@@ -137,11 +211,26 @@ can test the stricter behavior before it becomes required and migrate on purpose
137211

138212
Test every public deprecation with
139213
[`pytest.deprecated_call()`][pytest.deprecated_call]. Check the exact message
140-
and the replacement behavior. If deprecated code correctly calls another
141-
deprecated symbol, suppress only that expected inner
142-
[`DeprecationWarning`][]
143-
in a small [`warnings.catch_warnings()`][warnings.catch_warnings] block. The
144-
outer API must still emit its one public warning.
214+
and the replacement behavior. The outer API must still emit its one public
215+
warning, even when it silences inner ones as described above.
216+
217+
Check that the replacement doesn't go through anything deprecated with
218+
`frequenz.core.warnings.asserting_no_deprecations()`, rather than with an
219+
`"error"` filter. The filter turns the warning into an exception inside the
220+
code under test, where a broad `except` can swallow it; the helper records the
221+
warnings instead and fails when the block ends, listing each one and where it
222+
came from:
223+
224+
```python
225+
from frequenz.core.warnings import asserting_no_deprecations
226+
227+
from example import thing_from_proto2
228+
229+
230+
def test_thing_from_proto2_does_not_warn() -> None:
231+
with asserting_no_deprecations():
232+
assert thing_from_proto2(3) == "3"
233+
```
145234

146235
Add `RELEASE_NOTES.md` migration bullets that state the old behavior, the
147236
replacement, what changes, and the planned removal version. Remove the

‎docs/wrapping-guide/index.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ wrappers.
3030
`*_from_proto` and `*_to_proto` functions that translate protobuf messages
3131
to wrapper types.
3232
- [Deprecation and compatibility](deprecation-and-compatibility.md) — Describes
33-
how to replace public functions and tighten validation without surprising
34-
callers.
33+
how to replace or move public symbols and tighten validation without
34+
surprising callers, and how to keep the library's own deprecations quiet.
3535
- [Testing](testing.md) — Shows how to place tests, check enum parity, and make
3636
documentation examples and warnings part of the test suite.

‎docs/wrapping-guide/testing.md‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ Assert each expected deprecation with
4343
[`pytest.deprecated_call()`][pytest.deprecated_call]. This records the public
4444
warning and stops an unrelated warning from being hidden. When deprecated code
4545
correctly calls another deprecated symbol, suppress only that inner
46-
`DeprecationWarning` in a small warning block to avoid duplicate messages. In a
47-
test, use the same small suppression only for a warning that a dedicated
48-
assertion already checks. Never suppress warnings globally.
46+
`DeprecationWarning` in a small `frequenz.core.warnings.ignoring_deprecations()`
47+
block to avoid duplicate messages. In a test, use the same small suppression
48+
only for a warning that a dedicated assertion already checks, and check that a
49+
replacement doesn't warn with `frequenz.core.warnings.asserting_no_deprecations()`.
50+
Never suppress warnings globally, and never with
51+
[`warnings.catch_warnings()`][warnings.catch_warnings]; [Deprecation and
52+
compatibility](deprecation-and-compatibility.md) explains both helpers and why.

0 commit comments

Comments
 (0)