Skip to content

Commit 915e06e

Browse files
roed314claude
andcommitted
Address review: API contract wording and root API page
Four review points on the __all__ freeze, all in the contract and its documentation rather than the declarations themselves. 1. __version__ is unambiguously public. Versioning.md said membership in __all__ decides, then separately excluded every underscore-prefixed name with no exception, which classified the exported __version__ both ways. Both passages now apply the single test: a name is public exactly when it appears in its module's __all__, dunder or not. 2. The API reference covers the package root. docs/api/package.md documents psycodict itself; seven of its eight exports are imported members and __version__ is a special data member, so the page carries local imported-members/special-members options (the module pages keep documenting exactly their own __all__ -- verified: every page's top-level anchors now equal that module's __all__, and range_formatter, KeyedDefaultDict and number_types remain absent). __version__ gets a #: doc-comment so it renders with a description rather than str's docstring. Versioning.md's API-reference link becomes the absolute Read the Docs URL, since the canonical file is read on GitHub where docs/api/index.md does not exist. test_public_api.py gains a guard that every module in the frozen surface has an automodule page, and one that every exported callable has a docstring (without undoc-members, an undocumented export silently leaves the reference). The conf.py comment is corrected to claim only the upper bound that turning undoc-members off actually gives. 3. The changelog no longer implies __all__ is behavior-neutral. Explicit imports of a non-public name still resolve; `from ... import *` now binds __all__ and nothing else, which is a real behavior change and is described as one. test_star_import_gives_exactly_all is parametrized over every module in EXPECTED rather than the package root alone. 4. The API overview matches the collision-safe lookup policy: db[name] is canonical and db.<name> is shorthand for when the name is not shadowed. Searching.md's "equivalently" claim gets the same correction. Full suite 1439 passed, 36 skipped, 1 xfailed; ruff and the -W docs build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 756c3fa commit 915e06e

8 files changed

Lines changed: 141 additions & 33 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -463,16 +463,20 @@ hardening standalone use; the highlights:
463463
build. The release workflow's actions are pinned to commit SHAs, and the
464464
install smoke tests assert `importlib.metadata.version == __version__` and run
465465
`pip check` for both a plain and a binary-extra install.
466-
467466
- **The public API is defined by `__all__`.** Each module now declares the
468467
names psycodict promises to keep across 1.x, and the API reference documents
469-
exactly those (Sphinx `undoc-members` is off). A non-`__all__` name -- even one
470-
with a docstring -- is implementation: still importable, so nothing downstream
471-
breaks, but not part of the stability promise. `tests/test_public_api.py`
472-
freezes the surface so a change to it is deliberate. Versioning.md is rewritten
473-
around this, and states that `db[name]` is the canonical table lookup while
474-
`db.<name>` is convenience syntax a real database attribute wins over -- so
475-
adding a method in a minor release never makes a table unreachable through
468+
exactly those (Sphinx `undoc-members` is off). A name absent from `__all__` --
469+
even one with a docstring -- is implementation: not part of the stability
470+
promise, though **explicit imports of it still resolve**, so
471+
`from psycodict.utils import range_formatter` keeps working. *Wildcard*
472+
imports do change: `from psycodict.utils import *` now binds only the four
473+
curated names rather than every non-underscore module name, so a module that
474+
relied on `import *` to pull in a helper may bind fewer names than before.
475+
`tests/test_public_api.py` freezes the surface, and checks the wildcard
476+
behavior of every module, so a change to either is deliberate. Versioning.md is
477+
rewritten around this, and states that `db[name]` is the canonical table lookup
478+
while `db.<name>` is convenience syntax a real database attribute wins over --
479+
so adding a method in a minor release never makes a table unreachable through
476480
`db[name]`.
477481

478482
### Release candidates

‎Searching.md‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,11 @@ change without notice. Code outside psycodict should call only the
2222
non-underscore methods described below (on the table, and on its public
2323
`table.stats` attribute).
2424

25-
A search table is reached as `db.<table_name>` (equivalently `db["<table_name>"]`)
26-
on a `PostgresDatabase`. Every example in this document uses `db` for the
27-
database and a table variable such as `nf = db.nf_fields`.
25+
A search table is reached as `db["<table_name>"]` on a `PostgresDatabase`, and
26+
as `db.<table_name>` when the name is not shadowed by a real attribute or method
27+
of the database object (see [Versioning.md](Versioning.md#reaching-a-table)).
28+
Every example in this document uses `db` for the database and a table variable
29+
such as `nf = db.nf_fields`.
2830

2931
## Contents
3032

‎Versioning.md‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,18 @@ metadata tables living inside your database.
1010
## What is public
1111

1212
* **The names each module exports in its `__all__`**, and their documented
13-
behavior. These are exactly the names in the [API reference](api/index.md),
13+
behavior. A name is public exactly when it appears in its module's
14+
`__all__`; that single test decides it, with no exceptions. It covers
15+
explicitly exported special names such as `psycodict.__version__`, and any
16+
name *absent* from `__all__` — underscore-prefixed or not, docstring or not —
17+
is implementation: an explicit `from psycodict.utils import range_formatter`
18+
still resolves, but the name is not part of this promise and may change in
19+
any release. (`from psycodict.utils import *`, on the other hand, binds
20+
`__all__` and nothing else, so a wildcard import binds fewer names than it
21+
did before 1.0.) The `__all__` names are exactly the names in the
22+
[API reference](https://psycodict.readthedocs.io/en/latest/api/index.html),
1423
and the snapshot test `tests/test_public_api.py` freezes them, so the promise
15-
and the code cannot drift apart. A non-underscore name that is *not* in an
16-
`__all__` (and every underscore-prefixed name, whatever module it lives in)
17-
is implementation: it may still be importable, but it is not part of this
18-
promise and may change in any release. Having a docstring does not make a
19-
name public; being in `__all__` does.
24+
and the code cannot drift apart.
2025
* **The query language** as specified in [QueryLanguage.md](QueryLanguage.md):
2126
the meaning of a query dictionary is stable within a major version: new
2227
features may be added in minor versions, but functioning queries will
@@ -49,11 +54,12 @@ name.
4954

5055
## What is not covered
5156

52-
Non-`__all__` names, whether or not they carry a docstring; underscore-prefixed
53-
names; the exact SQL text psycodict emits (only its semantics); performance
54-
characteristics; the contents of log files; and undocumented behavior generally,
55-
even where observable. If something undocumented matters to your project, open
56-
an issue — turning it into documented (hence stable) behavior is usually easy.
57+
Names absent from their module's `__all__`, whether or not they carry a
58+
docstring and whatever they are named; the exact SQL text psycodict emits (only
59+
its semantics); performance characteristics; the contents of log files; and
60+
undocumented behavior generally, even where observable. If something
61+
undocumented matters to your project, open an issue — turning it into documented
62+
(hence stable) behavior is usually easy.
5763

5864
## Database metadata compatibility
5965

‎docs/api/index.md‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,17 @@
11
# API reference
22

3-
Generated from the docstrings. The map of the library:
3+
Generated from the docstrings. Each page documents exactly its module's
4+
`__all__` — the supported surface [Versioning.md](../Versioning.md) promises to
5+
keep across 1.x. The map of the library:
46

7+
- {mod}`psycodict` — the package root: `__version__` and the names re-exported
8+
for convenience ({class}`~psycodict.utils.DelayCommit` and the SQL
9+
composition classes `SQL`, `Identifier`, `Placeholder`, `Literal`,
10+
`Composable`, `Composed`).
511
- {mod}`psycodict.database` — {class}`~psycodict.database.PostgresDatabase`,
6-
the connection object; each table in the database is an attribute of it.
12+
the connection object; access a table canonically as `db[name]`. Attribute
13+
access `db.<name>` is shorthand for the same lookup when the table name is
14+
not shadowed by a real attribute or method of the database object.
715
- {mod}`psycodict.searchtable` —
816
{class}`~psycodict.searchtable.PostgresSearchTable`, the read API
917
(`search`, `lucky`, `lookup`, `count`, `random`, …) driven by the query
@@ -36,6 +44,7 @@ Generated from the docstrings. The map of the library:
3644
```{toctree}
3745
:maxdepth: 1
3846
47+
package
3948
database
4049
searchtable
4150
table

‎docs/api/package.md‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# psycodict
2+
3+
The package root. Its `__all__` is the version marker and the names psycodict
4+
re-exports for convenience, so that downstream code need not import them from a
5+
submodule or from the driver; everything else lives in the modules below.
6+
7+
The autodoc options here are deliberately wider than on the module pages: seven
8+
of the eight root exports are *imported* members (`imported-members`) and
9+
`__version__` is a special data member (`special-members`), so without them
10+
Sphinx would silently render an empty page for a module whose entire public
11+
surface is re-exports. The options are local to this page — the module pages
12+
keep documenting exactly their own `__all__`.
13+
14+
```{eval-rst}
15+
.. automodule:: psycodict
16+
:imported-members:
17+
:special-members: __version__
18+
```

‎docs/conf.py‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,14 @@
5252
# documentation conventions (INPUT:/OUTPUT: bullet blocks, EXAMPLES:: with
5353
# literal transcripts); those are plain reST, so autodoc renders them as-is.
5454
autodoc_member_order = "bysource"
55-
# No "undoc-members": the API reference documents exactly each module's __all__
56-
# (its supported public surface), not every non-underscore name that happens to
57-
# have -- or lack -- a docstring. Versioning.md defines the public API as the
58-
# __all__ names, so the reference and the promise stay in step.
55+
# No "undoc-members": with __all__ defined, autodoc considers only the names a
56+
# module exports, and this keeps it from documenting the ones it cannot -- so a
57+
# helper that merely carries a docstring no longer lands in the reference just
58+
# because it has one. That is an upper bound, not a proof of parity: autodoc
59+
# also *drops* an exported name it has no docstring for, and (on a page without
60+
# "imported-members"/"special-members") an exported name that is an import or a
61+
# dunder. tests/test_public_api.py supplies the parity half -- every __all__
62+
# name has a docstring, and every module in the frozen surface has a page here.
5963
autodoc_default_options = {
6064
"members": True,
6165
"show-inheritance": True,

‎psycodict/__init__.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@
3333

3434
# Single source of truth for the package version: pyproject.toml reads it via
3535
# ``[tool.setuptools.dynamic]``, and it works from an uninstalled checkout too.
36+
#: The version of psycodict, as a :pep:`440` string. For an installed copy it
37+
#: is the same value ``importlib.metadata.version("psycodict")`` reports.
3638
__version__ = "1.0.0rc2"
3739

3840
try:

‎tests/test_public_api.py‎

Lines changed: 68 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,23 @@
66
``__all__``. This test freezes those sets, so adding or removing a public name
77
is a deliberate, reviewed change rather than an accident -- and so that a name a
88
downstream project relies on cannot quietly leave the promise.
9+
10+
It also pins the two things that promise depends on: that ``from <mod> import *``
11+
binds exactly ``__all__`` (the one behavioral change declaring it makes), and
12+
that the API reference can actually document the whole of ``__all__`` -- every
13+
module has a page, and every exported callable has a docstring.
914
"""
1015
import importlib
16+
import inspect
17+
import re
18+
from pathlib import Path
1119

1220
import pytest
1321

1422

23+
_API_DOCS = Path(__file__).resolve().parent.parent / "docs" / "api"
24+
25+
1526
# The frozen public surface, module by module. Changing this is changing the
1627
# 1.x API contract: update Versioning.md and coordinate downstream in the same
1728
# breath.
@@ -55,14 +66,21 @@ def test_every_exported_name_resolves(modname):
5566
assert hasattr(mod, name), "%s exports %s, which does not exist" % (modname, name)
5667

5768

58-
def test_star_import_gives_exactly_all():
69+
@pytest.mark.parametrize("modname", sorted(EXPECTED))
70+
def test_star_import_gives_exactly_all(modname):
5971
"""
60-
``from psycodict import *`` binds exactly the root ``__all__`` names.
72+
``from <module> import *`` binds exactly that module's ``__all__``.
73+
74+
This is the one place where declaring ``__all__`` changes what Python does
75+
rather than only what psycodict promises: before, a wildcard import bound
76+
every non-underscore name in the module. Pinning it per module keeps the
77+
behavioral consequence of the contract visible, and keeps it from looking
78+
like only the package root is governed by ``__all__``.
6179
"""
6280
ns = {}
63-
exec("from psycodict import *", ns)
64-
bound = {k for k in ns if not k.startswith("__") or k == "__version__"}
65-
assert bound == EXPECTED["psycodict"]
81+
exec("from %s import *" % modname, ns)
82+
bound = set(ns) - {"__builtins__"}
83+
assert bound == EXPECTED[modname]
6684

6785

6886
def test_downstream_imports_still_resolve():
@@ -88,3 +106,48 @@ def test_downstream_imports_still_resolve():
88106
from psycodict.base import ( # noqa: F401
89107
_meta_indexes_cols, _meta_constraints_cols, _meta_tables_cols,
90108
)
109+
110+
111+
@pytest.mark.parametrize("modname", sorted(EXPECTED))
112+
def test_module_has_an_api_reference_page(modname):
113+
"""
114+
Every module in the frozen surface has a page in the API reference.
115+
116+
Versioning.md promises that the ``__all__`` names are exactly the names in
117+
the API reference; that promise is only keepable if a module cannot join
118+
the public surface without a page to be documented on. (Sphinx's ``-W``
119+
build then catches a page that is not in a toctree.)
120+
"""
121+
if not _API_DOCS.is_dir():
122+
pytest.skip("built without docs/ (%s)" % _API_DOCS)
123+
documented = set()
124+
for page in _API_DOCS.glob("*.md"):
125+
documented.update(
126+
re.findall(r"^\s*\.\.\s+automodule::\s*(\S+)\s*$", page.read_text(), re.M)
127+
)
128+
assert modname in documented, (
129+
"%s is in the public API but no docs/api/*.md documents it; add a page "
130+
"and put it in the toctree in docs/api/index.md" % modname
131+
)
132+
133+
134+
@pytest.mark.parametrize("modname", sorted(EXPECTED))
135+
def test_every_exported_name_is_documented(modname):
136+
"""
137+
Every exported class and function carries a docstring.
138+
139+
The API reference runs without ``undoc-members``, so an exported name with
140+
no docstring is silently absent from it -- the reference would quietly stop
141+
covering the whole of ``__all__``. (Exported data such as ``__version__``
142+
is documented by a ``#:`` comment, which lives in the source rather than on
143+
the object, so only callables are checked here.)
144+
"""
145+
mod = importlib.import_module(modname)
146+
for name in sorted(mod.__all__):
147+
obj = getattr(mod, name)
148+
if not (inspect.isclass(obj) or inspect.isroutine(obj)):
149+
continue
150+
assert obj.__doc__, (
151+
"%s.%s is exported but has no docstring, so the API reference "
152+
"would not document it" % (modname, name)
153+
)

0 commit comments

Comments
 (0)