Skip to content

feat(enrichment): add missing-deprecation rule#351

Closed
Alberto-Codes wants to merge 3 commits intomainfrom
feat/enrichment-35-2-missing-deprecation-rule
Closed

feat(enrichment): add missing-deprecation rule#351
Alberto-Codes wants to merge 3 commits intomainfrom
feat/enrichment-35-2-missing-deprecation-rule

Conversation

@Alberto-Codes
Copy link
Copy Markdown
Owner

docvet had no way to detect deprecated functions whose docstrings omit a deprecation notice, leaving callers unaware they should migrate. This adds the missing-deprecation enrichment rule to close that gap.

  • Add _check_missing_deprecation with scope-aware AST walk detecting warnings.warn(..., DeprecationWarning/PendingDeprecationWarning/FutureWarning) and all @deprecated decorator forms (bare, dotted, call)
  • Add require_deprecation_notice: bool = True to EnrichmentConfig, wire require-deprecation-notice config key through _VALID_ENRICHMENT_KEYS and format_config_toml
  • Add 25 unit tests using @pytest.mark.parametrize across all warning categories and decorator forms; cross-rule interaction test verifies no dispatch conflicts
  • Create docs/site/rules/missing-deprecation.md rule reference page, register in docs/rules.yml, add require-deprecation-notice to enrichment options table and complete example in docs/site/configuration.md

Test: uv run pytest tests/unit/checks/test_missing_deprecation.py -v


PR Review

Checklist

  • Self-reviewed my code
  • Tests pass (uv run pytest)
  • Lint passes (uv run ruff check .)
  • Types pass (uv run ty check)
  • Breaking changes use ! in title and BREAKING CHANGE: in body

Review Focus

  • Scope-aware stack walk in _has_deprecation_warning_call (AC6 — stops at nested FunctionDef/AsyncFunctionDef/ClassDef)
  • _has_deprecated_decorator covering all 4 forms including PEP 702 call forms
  • NOT added to _SPHINX_AUTO_DISABLE_RULES — detection is code-AST-based, style-agnostic

Related

  • Story 35.1 (missing-param-in-docstring / extra-param-in-docstring) established the _RULE_DISPATCH two-entry convention and parametrize mandate followed here

Adds a new enrichment rule that detects deprecated functions whose
docstrings omit a deprecation notice. Fires when a function calls
warnings.warn(..., DeprecationWarning/PendingDeprecationWarning/FutureWarning)
at its top scope or carries a @deprecated decorator (any form), and the
docstring does not contain the word "deprecated" (case-insensitive).
Scope-aware AST walk prevents false positives from nested defs/classes.
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 11, 2026

Codecov Report

❌ Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/docvet/checks/enrichment.py 96.07% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Alberto-Codes Alberto-Codes marked this pull request as ready for review March 11, 2026 20:12
Copilot AI review requested due to automatic review settings March 11, 2026 20:12
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new enrichment rule (missing-deprecation) to detect deprecated functions that don’t mention deprecation in their docstring, along with a config toggle, tests, and documentation updates.

Changes:

  • Implement missing-deprecation rule in enrichment (AST-based detection for warnings.warn(..., DeprecationWarning|PendingDeprecationWarning|FutureWarning) and @deprecated decorator forms).
  • Add require-deprecation-notice toggle to EnrichmentConfig, including TOML formatting + docs.
  • Add comprehensive unit tests and register the rule in the rules catalog + docs site.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/docvet/checks/enrichment.py Implements the missing-deprecation rule + wires it into enrichment dispatch.
src/docvet/config.py Adds require_deprecation_notice config field, valid key, and TOML rendering support.
tests/unit/checks/test_missing_deprecation.py New unit tests covering warning/decorator variants, nesting behavior, and config gating.
docs/rules.yml Registers the new rule in the canonical rules registry.
tests/unit/test_docs_infrastructure.py Updates expected rules count to reflect the new rule.
docs/site/rules/missing-deprecation.md New rule reference page.
docs/site/configuration.md Documents the new require-deprecation-notice option and updates the full example.
_bmad-output/implementation-artifacts/sprint-status.yaml Updates sprint/story status metadata.
_bmad-output/implementation-artifacts/35-2-missing-deprecation-enrichment-rule.md Adds story implementation artifact detailing ACs, tasks, and verification.
.claude/rules/gh-discussions.md Adds internal reference notes for GitHub Discussions GraphQL workflows.

Comment thread docs/rules.yml
Comment thread docs/rules.yml
- Add import warnings to fix example in docs/rules.yml (Copilot)
- Add missing-deprecation entry to _RULE_CATALOG in mcp.py; update
  unit and integration test counts 22→23 (Copilot)
- Add tests for chained-attribute and non-warnings-module warn calls
  to cover lines 1971/1973 in _is_warnings_warn_call (Codecov)
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

tests/unit/test_mcp.py:555

  • The MCP rule catalog tests are now updated to 23 rules, but the repo defines 25 rule ids in docs/rules.yml and there are implemented checks for additional rules (notably missing-returns and overload-has-docstring). If docvet_rules is intended to be comprehensive, these assertions should be updated to include the missing rule names and the expected count adjusted accordingly.
    def test_rule_catalog_has_expected_count(self):
        assert len(_RULE_CATALOG) == 23

    def test_rule_catalog_entries_have_required_keys(self):
        expected_keys = {
            "name",
            "check",
            "description",
            "category",
            "guidance",
            "fix_example",
        }
        for entry in _RULE_CATALOG:
            assert set(entry.keys()) == expected_keys

    def test_rule_catalog_names_match_emitted_rules(self):
        expected_rules = {
            # presence (1)
            "missing-docstring",
            # enrichment (13)
            "missing-raises",
            "missing-yields",
            "missing-receives",
            "missing-warns",
            "missing-other-parameters",
            "missing-attributes",
            "missing-typed-attributes",
            "missing-examples",
            "missing-cross-references",
            "prefer-fenced-code-blocks",
            "missing-param-in-docstring",
            "extra-param-in-docstring",
            "missing-deprecation",
            # freshness (5)
            "stale-signature",
            "stale-body",
            "stale-import",
            "stale-drift",
            "stale-age",
            # coverage (1)
            "missing-init",
            # griffe (3)
            "griffe-unknown-param",
            "griffe-missing-type",
            "griffe-format-warning",
        }
        catalog_names = {entry["name"] for entry in _RULE_CATALOG}
        assert catalog_names == expected_rules

Comment thread tests/integration/test_mcp.py
Comment thread src/docvet/mcp.py Outdated
Comment thread src/docvet/mcp.py
Comment thread src/docvet/checks/enrichment.py Outdated
Comment thread docs/rules.yml
- Update docs/rules.yml header comment 24→25 rules (Copilot)
- Simplify missing-deprecation fix_example in _RULE_CATALOG to
  docstring-only fragment for consistency with other enrichment entries;
  removes incomplete warnings.warn call (Copilot)
- Start _has_deprecation_warning_call traversal from node.body instead
  of ast.iter_child_nodes(node) so decorators/annotations are not
  walked; add explicit list[ast.AST] annotation for type safety (Copilot)
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

@@ -0,0 +1,214 @@
# GitHub Discussions API Reference

Working patterns for GitHub Discussions via `gh api graphql`. The REST API and `gh discussion` CLI command do NOT support discussions — GraphQL is the only path.
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file says “The REST API … do NOT support discussions — GraphQL is the only path.”, but later claims a REST endpoint works for reading discussions. Please reconcile these statements (either remove the REST claim or clarify what is/িসn’t supported and under which conditions).

Suggested change
Working patterns for GitHub Discussions via `gh api graphql`. The REST API and `gh discussion` CLI command do NOT support discussions — GraphQL is the only path.
Working patterns for GitHub Discussions via `gh api graphql`. For creating or updating discussions, the REST API and `gh discussion` CLI command do NOT support the required operations — GraphQL is the only path for write actions. Limited read-only access to discussions may be available via specific REST endpoints, as documented below.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +4
# GitHub Discussions API Reference

Working patterns for GitHub Discussions via `gh api graphql`. The REST API and `gh discussion` CLI command do NOT support discussions — GraphQL is the only path.

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR metadata focuses on adding the missing-deprecation enrichment rule, but this PR also adds an internal GitHub Discussions API reference under .claude/rules/. If this is intentional, please mention it in the PR description; otherwise, consider moving it to a separate PR to keep the change focused and easier to review/revert.

Copilot uses AI. Check for mistakes.
Comment on lines +34 to +45
## Key IDs

These are stable — no need to look them up each session:

- **Repository ID**: `R_kgDORLG_EQ`
- **Announcements category ID**: `DIC_kwDORLG_Ec4C3lBx`
- **General category ID**: `DIC_kwDORLG_Ec4C3lBy`
- **Ideas category ID**: `DIC_kwDORLG_Ec4C3lB0`
- **Polls category ID**: `DIC_kwDORLG_Ec4C3lB2`
- **Q&A category ID**: `DIC_kwDORLG_Ec4C3lBz`
- **Show and tell category ID**: `DIC_kwDORLG_Ec4C3lB1`

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These hard-coded repository/category/discussion node IDs are specific to this repository and will be wrong in forks or if categories are reorganized. Consider adding an explicit note that they are docvet-repo-specific (and/or include a short “how to look them up” snippet) so readers don’t treat them as universally stable.

Copilot uses AI. Check for mistakes.
@Alberto-Codes
Copy link
Copy Markdown
Owner Author

Closing in favor of #353 which covers the same feature with additional improvements: 34 tests (vs 25), CC refactoring via _category_name helper, enrichment check docs page fully updated (4 missing rules + 3 config keys), parametrized scope boundary tests, and all 7 code review findings addressed.

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.

2 participants