Skip to content

Check type issues with ty and fix problems it finds#100

Merged
jcarbaugh merged 2 commits into
mainfrom
APR-33-check-types-with-ty
May 21, 2026
Merged

Check type issues with ty and fix problems it finds#100
jcarbaugh merged 2 commits into
mainfrom
APR-33-check-types-with-ty

Conversation

@jcarbaugh
Copy link
Copy Markdown
Member

@jcarbaugh jcarbaugh commented May 20, 2026

Summary

Adds Astral's ty to the dev dependency group and clears every diagnostic it surfaced against the us package. After this change uv run ty check us reports All checks passed! (down from 8 diagnostics: 5 errors + 3 warnings).

What changed

Add ty as a dev tool

  • pyproject.toml / uv.lockty==0.0.38 in the dev group, alongside ruff, pytest, and pytz. Run with uv run ty check us.

Real fixes (preferred over suppression)

  • us/states.py — removed unused # type: ignore on import jellyfish. Restructured lookup() so fallback_key is no longer initialized to None and then reassigned to str — it is now only bound inside the fallback_func is not None branch, which is the only branch that reads it. Eliminates the None | str dict-key write that ty was correctly flagging.
  • us/cli/states.pystate.shapefile_urls() legitimately returns Optional[Dict[str, str]]; guard the iteration with a urls is not None check.
  • us/tests/test_us.py — removed unused # type: ignore directives on jellyfish and pytest; added the same shapefile_urls() None guard inside the (skipped) test_head; added assert state.fips is not None inside test_county_fips_prefixed_by_state to narrow Optional[str] before startswith and give a clearer failure message if the invariant ever breaks.

Narrow suppression

  • us/tests/test_us.py — the import requests inside the permanently @pytest.mark.skipped test_head is suppressed with # ty: ignore[unresolved-import] and a one-line note. requests is intentionally not declared as a dependency, so a real fix would mean adding a dev dependency for code that never runs.

Annotations left alone (on purpose)

  • State.fips: Optional[str] stays optional — obsolete states genuinely lack a FIPS code.
  • shapefile_urls() -> Optional[Dict[str, str]] stays optional — same reason. Callers narrow at the use site instead of changing the public contract.

Test plan

  • uv run ty check usAll checks passed!
  • uv run pytest — 35 passed, 1 skipped
  • uv run ruff checkAll checks passed!

Adds ty to the dev dependency group and clears every diagnostic it
surfaces against the `us` package:

- Drop unused `# type: ignore` directives on jellyfish and pytest.
- Restructure `lookup()` so `fallback_key` is only ever bound to a
  `str`, removing the false `None | str` dict-key write.
- Guard `state.shapefile_urls()` against `None` in the CLI and in the
  skipped network test before iterating.
- Narrow `state.fips` with an assert in `test_county_fips_prefixed_by_state`.
- Suppress the `requests` import inside the permanently-skipped
  `test_head` with `# ty: ignore[unresolved-import]`.

Closes APR-33.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jcarbaugh
Copy link
Copy Markdown
Member Author

Review notes:

  • us/states.pylookup() refactor. The interesting change: fallback_key = None is gone. Now fallback_key is only bound inside the if fallback_func is not None: branch. The later write site (_lookup_cache[fallback_key] = matched_state) is itself guarded by the same fallback_func is not None, so by control flow the variable is always defined when it's used. Worth a careful read to confirm there's no path I missed where the second guard could be true while the first wasn't.
  • us/cli/states.py:41 and us/tests/test_us.py:257shapefile_urls() is annotated Optional[Dict[str, str]] and only returns None when state.fips is falsy. In practice neither call site ever hits that branch (both iterate STATES_AND_TERRITORIES, which excludes obsolete states), so the guards are belt-and-suspenders but cost nothing.
  • us/tests/test_us.py:316 — added assert state.fips is not None inside test_county_fips_prefixed_by_state. Has the side effect of explicitly testing the invariant; if it ever breaks, the failure message is clearer than the previous AttributeError would have been.
  • # ty: ignore[unresolved-import] on requests — the only suppression in the diff. The test is @pytest.mark.skipped permanently and requests is not a project dependency. Open to a different approach (e.g., adding requests to dev) if you'd rather not carry the ignore comment.
  • State.fips and shapefile_urls() return type are intentionally left as Optional[...] — obsolete states really do lack FIPS codes, so flipping the field to str would lose information for downstream users.

@jcarbaugh jcarbaugh merged commit cf2f200 into main May 21, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant