Skip to content

Update GitHub Actions to latest and deprecate ACS class#300

Merged
chekos merged 2 commits into
mainfrom
claude/interesting-taussig
Mar 9, 2026
Merged

Update GitHub Actions to latest and deprecate ACS class#300
chekos merged 2 commits into
mainfrom
claude/interesting-taussig

Conversation

@chekos

@chekos chekos commented Mar 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Bump actions/checkout v4→v6, astral-sh/setup-uv v5→v7, actions/first-interaction v1→v3 across all workflows
  • Deprecate the legacy ACS class with a DeprecationWarning pointing users to get_pums() and get_acs()

This supersedes and should auto-close the open Dependabot PRs (#291, #290, #289, #287, #281).

Closes #5

Test plan

  • CI passes (lint + tests across Python 3.10-3.13)
  • Verify Dependabot PRs auto-close after merge

🤖 Generated with Claude Code

Bump actions/checkout v4→v6, astral-sh/setup-uv v5→v7, and
actions/first-interaction v1→v3 across all workflows. Deprecate the
legacy ACS class in favor of get_pums() and get_acs().

Closes #5

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Mar 9, 2026

Copy link
Copy Markdown

Claude finished @chekos's task in 2m 14s —— View job


PR Review

  • Gather context
  • Read changed files
  • Review code quality and best practices
  • Check for bugs, security, and performance issues
  • Post review feedback

Summary

This PR is clean and well-scoped. The deprecation of ACS is correct in intent, but there are a few issues worth addressing before merging.


Issues

1. stacklevel=2 is wrong in __post_init__ — warning points to wrong location

pypums/surveys.py:35

warnings.warn(
    "ACS() is deprecated. ...",
    DeprecationWarning,
    stacklevel=2,  # ❌ points to generated __init__, not user code
)

For a dataclass, the call stack when the user writes ACS() is:

user code: ACS(...)          ← stacklevel=3 (this is what we want)
  → generated __init__       ← stacklevel=2 (currently points here)
    → __post_init__          ← stacklevel=1
      → warnings.warn()

With stacklevel=2, the warning is attributed to the dataclass-generated __init__, not the user's call site. This makes the warning less helpful — IDEs and tools that surface deprecation warnings will show the wrong line.

Fix: use stacklevel=3 to point to the user's code.

Fix this →


2. Existing tests don't expect DeprecationWarning — they should

tests/test_pypums.py

The test file calls ACS() extensively but doesn't assert the deprecation warning is raised. With --strict-markers already in pytest config, if someone later adds filterwarnings = "error" these tests will break. More importantly, the deprecation behavior is now untested.

Consider wrapping ACS() calls with pytest.warns(DeprecationWarning):

def test_acs_class_urls():
    with pytest.warns(DeprecationWarning, match="ACS\\(\\) is deprecated"):
        assert ACS()._survey_url == "https://..."

Or at minimum, add a dedicated test for the deprecation warning itself.

Fix this →


3. Sphinx .. deprecated:: directive is missing the version number

pypums/surveys.py:24

"""
.. deprecated::
    Use :func:`pypums.get_pums` ...

The Sphinx convention requires a version argument:

"""
.. deprecated:: X.Y.Z
    Use :func:`pypums.get_pums` ...

Without it, the rendered docs won't show which version introduced the deprecation. This is minor but good to have.


4. Action version bumps are unusually large — worth double-checking

The PR bumps:

  • actions/checkout v4 → v6
  • astral-sh/setup-uv v5 → v7
  • actions/first-interaction v1 → v3

These are large version jumps. If these versions exist and are stable, great — but it's worth verifying they're published on the GitHub Marketplace before merging, since a non-existent version would silently break all CI workflows.


What looks good

  • DeprecationWarning (not UserWarning) is the correct category per Python docs
  • ACS is still exported from pypums/__init__.py, preserving backwards compatibility
  • The warning message is clear and actionable
  • Using warnings module (not print) is the right approach

Address PR review: use stacklevel=3 so the warning points to user
code (not the dataclass __init__), add version to Sphinx deprecated
directive, and add explicit test for the DeprecationWarning.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Mar 9, 2026

Copy link
Copy Markdown

Claude finished @chekos's task in 1m 7s —— View job


PR Review (Updated)

  • Gather context
  • Read changed files
  • Check if previous review issues were addressed
  • Post updated review feedback

Summary

The follow-up commit ("Fix deprecation warning stacklevel, add version, and test coverage") cleanly addresses all three code-quality issues raised in the previous review. This PR is now in good shape to merge.


Previous Issues — All Resolved ✓

1. stacklevel=3 — Fixed
pypums/surveys.py:39 now correctly uses stacklevel=3, so the deprecation warning points to the user's call site rather than the dataclass-generated __init__. Good fix.

2. Tests updated for DeprecationWarning — Fixed
All existing ACS() test functions now carry @pytest.mark.filterwarnings("ignore::DeprecationWarning"), and a dedicated test was added:

def test_acs_class_emits_deprecation_warning():
    with pytest.warns(DeprecationWarning, match="ACS\\(\\) is deprecated"):
        ACS()

This is exactly the right approach — the warning behavior is now explicitly tested.

3. Sphinx .. deprecated:: version — Fixed
pypums/surveys.py:24 now reads .. deprecated:: 0.2, making the rendered docs useful.


Remaining Note (non-blocking)

The action version bumps (actions/checkout v4→v6, astral-sh/setup-uv v5→v7, actions/first-interaction v1→v3) are still large jumps. If CI passes on this branch, that's the best confirmation they exist and work. Worth checking the CI run results before merging.


What looks good

  • DeprecationWarning (correct category), warning message is clear and actionable
  • ACS still exported from __init__.py — backwards-compatible
  • Test hygiene is clean: existing tests suppress the warning via marker, and a dedicated test asserts the warning is raised
  • Sphinx docstring convention followed correctly

This PR is ready to merge once CI is green.

@chekos chekos merged commit b1b3d23 into main Mar 9, 2026
13 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.

fix __repr__ for invalid surveys

1 participant