feat: fully type donfig and ship py.typed by maxrjones · Pull Request #149 · pytroll/donfig

maxrjones · 2026-07-24T18:11:04Z

This is PR three out of four in a sequence to support downstream use cases. It improves typing via:

complete type annotations (which caught the bug in fix: Config.merge always raised AttributeError #148)
strict type checking via pre-commit and CI
donfig/py.typed so that downstream users' type checkers can see donfig's types

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

codecov · 2026-07-24T18:12:24Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.46%. Comparing base (7b90e4d) to head (d2ccb82).
⚠️ Report is 87 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
+ Coverage   98.28%   98.46%   +0.18%     
==========================================
  Files           6        6              
  Lines         759      784      +25     
==========================================
+ Hits          746      772      +26     
+ Misses         13       12       -1

Flag	Coverage Δ
unittests	`98.46% <100.00%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

djhoese

This looks good to me. Thanks for doing it. I'm not sure I entirely understand or am comfortable with the changes to self.defaults, but I think I mostly have problems with the previously allowed behavior of in-place modifications. I left some comments inline.

djhoese · 2026-07-24T18:32:27Z

-    ):
+        config: MutableMapping[str, Any],
+        lock: SerializableLock | contextlib.AbstractContextManager[Any],
+        deprecations: Mapping[str, str | None],


Hm I wonder why this was a MutableMapping before. Good catch.

…act explicit

djhoese · 2026-07-27T17:25:12Z

Any other changes needed on this or ready for merge?

maxrjones · 2026-07-27T21:30:08Z

Any other changes needed on this or ready for merge?

I pushed a couple clean up changes. I think this PR is now ready for merge.

djhoese · 2026-07-28T14:54:37Z

Ah sorry, I merged your other PR first and now there are conflicts. Do you mind fixing those? Rebasing would be nice if you're feeling brave 😉

djhoese · 2026-07-28T14:56:21Z

Just curious, what lead to the simplification/restriction to dict in some of those last changes? General philosophy I should keep in mind? I don't disagree with 95% of the changes, but the other 5% didn't seem strictly necessary (although I'm fine keeping them). Just curious.

maxrjones · 2026-08-03T14:57:29Z

hey, sorry for stepping back from this - I was busy with ESIP last week.

Just curious, what lead to the simplification/restriction to dict in some of those last changes? General philosophy I should keep in mind? I don't disagree with 95% of the changes, but the other 5% didn't seem strictly necessary (although I'm fine keeping them). Just curious.

I considered three options here:

Keep old: MutableMapping[str, Any] -> Mapping[str, Any] (what the annotations said before).
A TypeVar bound to MutableMapping (def update(old: T, ...) -> T), which is maximally faithful.
Plain dict -> dict, which is what dask's config.update does.

I went with 3 mainly because it matched dask's choice, so it seemed battle-tested, but there were a couple of supporting reasons:

The old return type of Mapping was unhelpful since the function returns the (mutated) old you passed in, and every real caller treats the result as a dict. Returning Mapping forces typed callers to cast or lose mutability. Narrowing a return type is always compatible.
Typing old as dict is what lets Config.config and to_dict() be dict[str, Any] instead of MutableMapping, which is the type downstream users probably want to see there.
It's honest about the implementation below the root: nested merges do old[k] = {}, so any nested structure comes out as plain dicts regardless of what container you passed in.
It's not a breaking change in practice since donfig only ships py.typed as of this branch, so type checkers never consumed the old MutableMapping annotation. Runtime behavior is the same before and after the PR.

In general TypeVar is the true "correct" abstract option, but I went with dict because it was simpler for a code-base that's introducing a lot more typing. The generalization is that read-only parameters get the widest type that works (Mapping), mutate-and-return values get the concrete type everyone uses (dict) unless a TypeVar earns its keep.

djhoese · 2026-08-03T15:01:38Z

I'm ok with the dict usage for now. I agree that TypeVar would be nice to have but maybe in a future PR/release.

maxrjones · 2026-08-03T16:18:17Z

-def test_paths_accepts_any_sequence(monkeypatch):
-    monkeypatch.setenv("MYPKG_CONFIG", "foo-bar")
-    config = Config("mypkg", paths=("/etc/mypkg",))
-    assert config.paths == ["/etc/mypkg", "foo-bar"]


Removed because it asserted tuple support, which contradicts the declared paths: list[str]

Right, but is there a reason Sequence wouldn't work?

Sequence + a runtime check against a bare str (which is natively a Sequence) would work. I can change this if you prefer that option?

Nah, let's leave it for now. This PR has seen enough work.

Side question: Have I been doing too much agentic coding or am I talking to your AI in these comments?

I have typed my comments to you with my human digits 😆

I do often run my responses by Claude first with an "is this accurate?" which might be injecting some bot-ness into my phrasing; I should probably cut that out and just be more confident.

At the same time, I have a long history of failing the "am I a bot tests" when trying to sign up for accounts (I still cannot get into facebook marketplace 😞), so it might not actually be anything different that I'm doing now...

Haha perfect

djhoese · 2026-08-03T19:19:12Z

I've lost count. Do you have more PRs planned? Then I assume you'd like a release after that?

maxrjones and others added 7 commits July 1, 2026 15:21

types: make donfig strict-clean and run mypy via a dependency group

595555d

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Merge branch 'main' into fix-typing

a721084

chore: type-check in CI

afe6465

chore: test config.merge

db46b35

chore: add py.typed

418dcbd

Merge branch 'main' into full-typing

d8cd00a

chore: scope ignore_missing_imports to cloudpickle only

301ba5a

maxrjones commented Jul 24, 2026

View reviewed changes

Comment thread donfig/config_obj.py Outdated

Configure mypy job

56d68ba

djhoese reviewed Jul 24, 2026

View reviewed changes

Comment thread pyproject.toml

djhoese reviewed Jul 24, 2026

View reviewed changes

djhoese added the enhancement New feature or request label Jul 24, 2026

djhoese self-assigned this Jul 24, 2026

refactor: type defaults as MutableSequence to make the aliasing contr…

973f111

…act explicit

maxrjones mentioned this pull request Jul 27, 2026

fix: copy caller-supplied defaults instead of aliasing #150

Merged

Merge branch 'main' into full-typing

0f3820c

types: simplify to dict

3d09a7c

djhoese added 2 commits August 3, 2026 09:39

Merge branch 'main' into full-typing

dc8a8aa

Add missing type annotations in test_config.py

511cdae

djhoese reviewed Aug 3, 2026

View reviewed changes

Comment thread donfig/config_obj.py

djhoese reviewed Aug 3, 2026

View reviewed changes

Comment thread donfig/config_obj.py

chore: update tests

d2ccb82

maxrjones commented Aug 3, 2026

View reviewed changes

djhoese merged commit 3ffae21 into pytroll:main Aug 3, 2026
13 checks passed

maxrjones deleted the full-typing branch August 3, 2026 19:19

Uh oh!

Conversation

maxrjones commented Jul 24, 2026

Uh oh!

codecov Bot commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

djhoese left a comment

Choose a reason for hiding this comment

Uh oh!

djhoese Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

djhoese commented Jul 27, 2026

Uh oh!

maxrjones commented Jul 27, 2026

Uh oh!

djhoese commented Jul 28, 2026

Uh oh!

djhoese commented Jul 28, 2026

Uh oh!

Uh oh!

Uh oh!

maxrjones commented Aug 3, 2026

Uh oh!

djhoese commented Aug 3, 2026

Uh oh!

maxrjones Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

djhoese Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

maxrjones Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

djhoese Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

maxrjones Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

djhoese Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

djhoese commented Aug 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 24, 2026 •

edited

Loading