Skip to content

refine(audit): flag inner-order @property @deprecated + strict property export#201

Merged
Borda merged 13 commits into
mainfrom
fix/property
Jun 23, 2026
Merged

refine(audit): flag inner-order @property @deprecated + strict property export#201
Borda merged 13 commits into
mainfrom
fix/property

Conversation

@Borda

@Borda Borda commented Jun 22, 2026

Copy link
Copy Markdown
Owner
Before submitting
  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

  • Add inner_order_property: bool field to DeprecationWrapperInfo; find_deprecation_wrappers sets it when a plain property (not _DeprecatedProperty) has a @deprecated-wrapped fget — fget-only warning, setter/deleter silently unprotected
  • Add _StrictProperty subclass of builtin property; raises TypeError at class-body time when handed an already-@deprecated getter (inner-order detection); exported as property from deprecate.__init__ for opt-in strict mode via from deprecate import property
  • 8 new tests covering flag detection, outer-order not-flagged, strict TypeError, normal usage, outer-order works, issubclass contract
  • Update properties.md, troubleshooting.md (CI snippet + strict import example), llms.txt with inner-order audit flag and strict import note

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

Borda and others added 2 commits June 22, 2026 12:56
… export

- Add `inner_order_property: bool` field to `DeprecationWrapperInfo`; `find_deprecation_wrappers` sets it when a plain `property` (not `_DeprecatedProperty`) has a `@deprecated`-wrapped `fget` — fget-only warning, setter/deleter silently unprotected
- Add `_StrictProperty` subclass of builtin `property`; raises `TypeError` at class-body time when handed an already-`@deprecated` getter (inner-order detection); exported as `property` from `deprecate.__init__` for opt-in strict mode via `from deprecate import property`
- 8 new tests covering flag detection, outer-order not-flagged, strict TypeError, normal usage, outer-order works, `issubclass` contract
- Update `properties.md`, `troubleshooting.md` (CI snippet + strict import example), `llms.txt` with inner-order audit flag and strict import note

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
- Add `phmdoctest:skip` to CI-template snippet in troubleshooting.md that used undefined `my_package`
- Add rule to CONTRIBUTING.md: runnable examples use `from tests import collection_X as my_package`; CI templates add `# phmdoctest:skip — CI template: replace my_package with your actual package`
- Add same rule to AGENTS.md Never section so agents do not repeat the mistake

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@Borda Borda requested a review from Copilot June 22, 2026 11:04
@dosubot dosubot Bot added documentation Improvements or additions to documentation enhancement New feature or request tests labels Jun 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds opt-in “strict property” support and audit visibility for the common footgun where @property is applied outside @deprecated (inner order), which otherwise only wraps fget and can leave setter/deleter paths silently unprotected. This fits pyDeprecate’s audit/CI tooling goal by making the pattern detectable (inner_order_property) and optionally fail-fast via from deprecate import property.

Changes:

  • Add inner_order_property: bool to DeprecationWrapperInfo and set it during class scanning when a plain property has a @deprecated-wrapped fget.
  • Introduce _StrictProperty (exported as deprecate.property) that raises TypeError when handed an already-deprecated getter (inner-order guard).
  • Add tests for the new audit flag and strict property behavior; update docs (properties.md, troubleshooting.md, llms.txt) plus agent/docs guidance.

Reviewed changes

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

Show a summary per file
File Description
src/deprecate/deprecation.py Adds _StrictProperty subclass that rejects inner-order @property @deprecated at definition time.
src/deprecate/audit.py Adds inner_order_property field and flags inner-order properties during class member scanning.
src/deprecate/__init__.py Re-exports _StrictProperty as property for opt-in strict mode (from deprecate import property).
tests/unittests/test_property.py Adds strict-property unit tests (inner-order TypeError, outer-order works, subclass contract).
tests/unittests/test_audit.py Adds tests for inner-order audit flag behavior and dataclass replace preservation.
docs/guide/properties.md Documents strict-mode import and the inner_order_property audit flag.
docs/troubleshooting.md Adds CI filtering snippet for inner_order_property and strict-mode example.
docs/llms.txt Updates AI-facing contract with inner-order detection and strict-mode import guidance.
AGENTS.md Adds guidance on my_package placeholders in runnable docs examples.
.github/CONTRIBUTING.md Mirrors the my_package placeholder guidance for docs examples.

Comment thread docs/guide/properties.md
Comment thread tests/unittests/test_audit.py
Comment thread tests/unittests/test_audit.py
Comment thread docs/guide/properties.md
@kilo-code-bot

kilo-code-bot Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
tests/unittests/test_audit.py 1043 Trailing comma before closing ); drop it to match the repo's test-file style
Files Reviewed (1 file)
  • tests/unittests/test_audit.py — 1 previous issue re-verified (trailing comma at line 1043); the shared-class __module__ mutation concern previously flagged around line 970 is resolved by switching to monkeypatch.setattr
Previous Review Summaries (3 snapshots, latest commit 7f03957)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 7f03957)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 1
WARNING 0
SUGGESTION 0
Issue Details (click to expand)

CRITICAL

File Line Issue
docs/guide/properties.md 138 Missing # phmdoctest:skip — CI make docs-tests is currently FAILING
Files Reviewed (7 changed files)
  • docs/guide/properties.md — 1 issue
  • src/deprecate/__init__.py
  • src/deprecate/audit.py
  • src/deprecate/deprecation.py
  • tests/unittests/test_audit.py
  • tests/unittests/test_property.py
  • docs/troubleshooting.md

Fix these issues in Kilo Cloud

Previous review (commit 04f9065)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 1
WARNING 0
SUGGESTION 0
Issue Details (click to expand)

CRITICAL

File Line Issue
docs/guide/properties.md 138 Missing # phmdoctest:skip — CI make docs-tests is currently FAILING
Files Reviewed (7 changed files)
  • docs/guide/properties.md — 1 issue
  • src/deprecate/__init__.py
  • src/deprecate/audit.py
  • src/deprecate/deprecation.py
  • tests/unittests/test_audit.py
  • tests/unittests/test_property.py
  • docs/troubleshooting.md

Fix these issues in Kilo Cloud

Previous review (commit a9384c1)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 1
WARNING 0
SUGGESTION 0
Issue Details (click to expand)

CRITICAL

File Line Issue
docs/guide/properties.md 138 Missing # phmdoctest:skip — CI make docs-tests is currently FAILING
Files Reviewed (7 changed files)
  • docs/guide/properties.md — 1 issue
  • src/deprecate/__init__.py
  • src/deprecate/audit.py
  • src/deprecate/deprecation.py
  • tests/unittests/test_audit.py
  • tests/unittests/test_property.py
  • docs/troubleshooting.md

Fix these issues in Kilo Cloud


Reviewed by step-3.7-flash-20260528 · Input: 244.1K · Output: 32.9K · Cached: 1.3M

Borda and others added 4 commits June 23, 2026 16:28
- docs/guide/properties.md: add `# phmdoctest:skip — TypeError raised at class-body time` as first line of inner-order example block; `make docs-tests` was generating `test_code_138` which raised `TypeError` during class-body evaluation
- tests/unittests/test_property.py: change `# type: ignore[prop-decorator]` → `# type: ignore[arg-type]`; mypy raises `[arg-type]` not `[prop-decorator]`, causing pre-commit.ci mypy hook to reject the suppression

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…or message, fget-only docstring

- src/deprecate/__init__.py: remove `"property"` from `__all__`; the alias remains importable as `from deprecate import property` (explicit opt-in) but star-import consumers (`from deprecate import *`) no longer receive the builtin shadow silently; add `# noqa: F401` to document the intentional export-without-__all__ pattern
- src/deprecate/deprecation.py: extend `_StrictProperty.__init__` TypeError message to include import origin hint (`deprecate._StrictProperty` / `from deprecate import property`) so authors who inherited the import via conftest or upstream `__init__` can trace the source; add one sentence to `fget` Args entry clarifying guard fires on fget only

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
- docs/guide/properties.md: change "every inner-order property" → "every inner-order `@property`"; `@cached_property @deprecated` inner-order is not flagged (no setter/deleter protocol), so "every" was inaccurate
- tests/unittests/test_audit.py: save and restore `InnerOrderDeprecatedPropCls.__module__` in `test_inner_order_property_flagged` via try/finally; the mutation to a shared fixture class was not restored and could affect subsequent tests reading `__module__`
- tests/unittests/test_audit.py: remove trailing comma before `)` in `test_inner_order_flag_survives_dataclass_replace` to match project style convention

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 10 out of 10 changed files in this pull request and generated 2 comments.

Comment thread src/deprecate/deprecation.py
Comment thread docs/troubleshooting.md
Borda and others added 6 commits June 23, 2026 17:48
- docs/guide/audit.md: add `inner_order_property=False` to both `DeprecationWrapperInfo` repr lines in the validate_deprecation_wrapper example output block; the new field appears in repr() after adding it to the dataclass in PR #201 and was not reflected in the hardcoded snapshot

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…ssage

- src/deprecate/deprecation.py: change `deprecate._StrictProperty` → `deprecate.deprecation._StrictProperty` in the TypeError message; the class lives in `deprecate.deprecation` and `deprecate._StrictProperty` does not resolve — caught by @Copilot review comment

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…with monkeypatch

- AGENTS.md + CONTRIBUTING.md: reword "never use bare import my_package" rule — the real constraint is never importing a fictional package name; runnable examples must use actual test collection modules (collection_deprecate, collection_misconfigured, collection_chains)
- tests/unittests/test_audit.py: replace manual orig_module save/restore try/finally with monkeypatch.setattr — teardown now handled by pytest fixture, not inline finally block

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…ttr across audit tests

All 14 test methods in TestFindDeprecationWrappersClassScan and TestInnerOrderPropertyAudit
that mutated OldCls.__module__ directly now use monkeypatch.setattr so pytest restores the
attribute automatically after each test. Added inline comments explaining why both lines are
needed: scanner filters by __module__, and the mod attribute injection makes inspect.getmembers()
find the class.

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…ttr across audit tests

All 14 test methods in TestFindDeprecationWrappersClassScan and TestInnerOrderPropertyAudit
that mutated OldCls.__module__ directly now use monkeypatch.setattr so pytest restores the
attribute automatically after each test. Added inline comments explaining why both lines are
needed: scanner filters by __module__, and the mod attribute injection makes inspect.getmembers()
find the class.

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@Borda Borda changed the title feat(audit): flag inner-order @property @deprecated + strict property export chore(audit): flag inner-order @property @deprecated + strict property export Jun 23, 2026
@Borda Borda merged commit a8ad93f into main Jun 23, 2026
49 checks passed
@Borda Borda deleted the fix/property branch June 23, 2026 20:23
@Borda Borda changed the title chore(audit): flag inner-order @property @deprecated + strict property export refine(audit): flag inner-order @property @deprecated + strict property export Jun 23, 2026
Borda added a commit that referenced this pull request Jun 23, 2026
- Add `[UnReleased]` entries for #201 (inner_order_property flag, _StrictProperty export) and #200 (circular chain RuntimeError, _StrictProperty module path fix)
- Add CHANGELOG update rule to AGENTS.md `Always:` block: write entry at PR-creation time with `(#N)` link; skip docs/tests/CI/refactoring
- Add matching checklist item to CONTRIBUTING.md "Before Opening a PR"

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants