[Grammar] Phase 2.2: Refactor canonical_grammar.py to delegate to unified_grammar #2836
Conversation
❌ Deploy Preview for stunning-zabaione-f1f1ef failed. Why did it fail? →
|
Co-authored-by: fermga <203334638+fermga@users.noreply.github.com>
Automated Code ReviewPlease review the workflow logs for details. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| import warnings | ||
|
|
||
| import pytest |
Check notice
Code scanning / CodeQL
Unused import Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the problem, we should simply remove the unused import pytest statement at line 9 from the file tests/unit/operators/test_canonical_grammar_legacy.py. This will ensure there is no unnecessary dependency or line of code present. No additional changes are necessary, as the rest of the code does not require pytest as a direct import. The fix only requires deleting the single line.
| @@ -6,7 +6,6 @@ | ||
|
|
||
| import warnings | ||
|
|
||
| import pytest | ||
|
|
||
| from tnfr.operators.definitions import ( | ||
| Emission, |
| from tnfr.operators.definitions import ( | ||
| Emission, | ||
| Reception, | ||
| Coherence, | ||
| Dissonance, | ||
| Mutation, | ||
| Silence, | ||
| Coupling, | ||
| SelfOrganization, | ||
| ) |
Check notice
Code scanning / CodeQL
Unused import Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
The best way to fix the issue is to remove SelfOrganization from the import statement on line 11. This preserves all other required imports from tnfr.operators.definitions and eliminates the unused import, making the code cleaner and slightly faster to load. We should only edit the import line(s) that declare this symbol; no other changes are required.
| @@ -16,7 +16,6 @@ | ||
| Mutation, | ||
| Silence, | ||
| Coupling, | ||
| SelfOrganization, | ||
| ) | ||
|
|
||
|
|
| warnings.simplefilter("always") | ||
| # Re-import to trigger warning | ||
| import importlib | ||
| import tnfr.operators.canonical_grammar |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the problem, remove all instances of from tnfr.operators.canonical_grammar import CanonicalGrammarValidator from the file and add a single import tnfr.operators.canonical_grammar at the top (or reuse the one inside the scope). Wherever CanonicalGrammarValidator is used, replace it with tnfr.operators.canonical_grammar.CanonicalGrammarValidator. This maintains current behavior and achieves the same warning suppression by controlling when the module is imported. All references to CanonicalGrammarValidator are local, so we do not need to add any new imports or initializations. Only the relevant function bodies (and possibly the import statement in the concerning test method) need updating. No other definitions are needed.
| @@ -50,13 +50,13 @@ | ||
| # Import with suppressed import warning | ||
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings('ignore', category=DeprecationWarning) | ||
| from tnfr.operators.canonical_grammar import CanonicalGrammarValidator | ||
| import tnfr.operators.canonical_grammar | ||
|
|
||
| ops = [Emission(), Coherence(), Silence()] | ||
|
|
||
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| result = CanonicalGrammarValidator.validate_initialization(ops) | ||
| result = tnfr.operators.canonical_grammar.CanonicalGrammarValidator.validate_initialization(ops) | ||
|
|
||
| # Check warning | ||
| assert len(w) > 0 | ||
| @@ -71,13 +66,13 @@ | ||
| """validate_convergence() should emit DeprecationWarning.""" | ||
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings('ignore', category=DeprecationWarning) | ||
| from tnfr.operators.canonical_grammar import CanonicalGrammarValidator | ||
| import tnfr.operators.canonical_grammar | ||
|
|
||
| ops = [Emission(), Dissonance(), Coherence(), Silence()] | ||
|
|
||
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| result = CanonicalGrammarValidator.validate_convergence(ops) | ||
| result = tnfr.operators.canonical_grammar.CanonicalGrammarValidator.validate_convergence(ops) | ||
|
|
||
| assert len(w) > 0 | ||
| assert issubclass(w[0].category, DeprecationWarning) | ||
| @@ -131,13 +121,13 @@ | ||
| """validate() should emit DeprecationWarning.""" | ||
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings('ignore', category=DeprecationWarning) | ||
| from tnfr.operators.canonical_grammar import CanonicalGrammarValidator | ||
| import tnfr.operators.canonical_grammar | ||
|
|
||
| ops = [Emission(), Coherence(), Silence()] | ||
|
|
||
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| is_valid, messages = CanonicalGrammarValidator.validate(ops) | ||
| is_valid, messages = tnfr.operators.canonical_grammar.CanonicalGrammarValidator.validate(ops) | ||
|
|
||
| assert len(w) > 0 | ||
| assert issubclass(w[0].category, DeprecationWarning) |
There was a problem hiding this comment.
Pull Request Overview
This PR deprecates the canonical_grammar.py module in favor of unified_grammar.py, consolidating the previously separate RC1-RC4 canonical rules into the unified U1-U4 constraint system. The legacy module is maintained for backward compatibility with all methods delegating to the unified grammar, and deprecation warnings are emitted on import and method usage.
- Replaced the implementation of
canonical_grammar.pywith a legacy wrapper that delegates tounified_grammar.py - Added comprehensive test coverage for the legacy module to ensure deprecation warnings are emitted correctly
- Updated all operator set exports and method delegates to maintain backward compatibility
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
tests/unit/operators/test_canonical_grammar_legacy.py |
New test file providing comprehensive coverage for the deprecated canonical_grammar module, validating deprecation warnings and backward compatibility |
src/tnfr/operators/canonical_grammar.py |
Converted from implementation to legacy wrapper; now imports and delegates to unified_grammar with deprecation warnings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Old Canonical Grammar (RC1-RC4) → Unified Grammar (U1-U4) | ||
| ---------------------------------------------------------- | ||
| RC1: Initialization → U1a: Initiation | ||
| RC2: Convergence → U2: CONVERGENCE & BOUNDEDNESS |
There was a problem hiding this comment.
Trailing whitespace detected on line 16. Remove the extra spaces after 'BOUNDEDNESS'.
| RC2: Convergence → U2: CONVERGENCE & BOUNDEDNESS | |
| RC2: Convergence → U2: CONVERGENCE & BOUNDEDNESS |
| # Re-import to trigger warning | ||
| import importlib | ||
| import tnfr.operators.canonical_grammar | ||
| importlib.reload(tnfr.operators.canonical_grammar) |
There was a problem hiding this comment.
The reload approach may not reliably trigger the module-level warning in all test scenarios. Consider using sys.modules.pop() before import or importing with a different approach to ensure the warning is always triggered fresh in the test.
|
|
||
| # Check that a warning was emitted | ||
| deprecation_warnings = [ | ||
| warning for warning in w |
There was a problem hiding this comment.
Trailing whitespace on line 37 after 'w'. Remove the extra space.
| warning for warning in w | |
| warning for warning in w |
| warnings.simplefilter("always") | ||
| # Re-import to trigger warning | ||
| import importlib | ||
| import tnfr.operators.canonical_grammar |
There was a problem hiding this comment.
Module 'tnfr.operators.canonical_grammar' is imported with both 'import' and 'import from'.
| ) | ||
|
|
||
| if TYPE_CHECKING: | ||
| from ..types import NodeId |
There was a problem hiding this comment.
Import of 'NodeId' is not used.
| from ..types import NodeId |
| import pytest | ||
|
|
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
| Reception, | ||
| Coherence, | ||
| Dissonance, | ||
| Mutation, | ||
| Silence, | ||
| Coupling, | ||
| SelfOrganization, |
There was a problem hiding this comment.
Import of 'SelfOrganization' is not used.
Import of 'Reception' is not used.
| Reception, | |
| Coherence, | |
| Dissonance, | |
| Mutation, | |
| Silence, | |
| Coupling, | |
| SelfOrganization, | |
| Coherence, | |
| Dissonance, | |
| Mutation, | |
| Silence, | |
| Coupling, |
Pull Request: Refactor canonical_grammar.py to Legacy Alias
🎯 Intent
Eliminate duplication between RC1-RC4 (canonical_grammar.py) and U1-U4 (unified_grammar.py) constraint systems. Single source of truth maintained while preserving backward compatibility.
🔧 Changes
Type of Change:
Core Changes:
canonical_grammar.pywith delegation tounified_grammar.pyDeprecationWarningon all legacy APIsGENERATOR_OPS→GENERATORS, etc.Constraint Mapping:
New Test Coverage:
test_canonical_grammar_legacy.pyverify deprecation warnings and functional equivalence🔬 Structural Impact
Operators Involved:
All operator grammar validation (emission, coherence, dissonance, mutation, coupling, resonance, self_organization, silence, transition, recursivity)
Affected Invariants:
None - validation logic unchanged, only refactored for consolidation
Metrics Impact:
✅ Quality Checklist
Code Quality:
.pyistub files generated/updated (no changes to type signatures)TNFR Canonical Requirements:
Testing:
Documentation:
docs/changelog.d/)Security (if applicable):
make security-audit)🧪 Testing Evidence
Test Coverage:
File Size Impact:
🔗 Related Issues
Closes #2828 (Grammar Phase 2.2)
📋 Additional Context
Migration Timeline: Users have until version 8.0.0 to migrate. All existing code continues working with deprecation warnings.
Benefits:
Reviewer Notes
Original prompt
This section details on the original issue you should resolve
<issue_title>[Grammar] Phase 2.2: Update canonical_grammar.py to use unified_grammar and mark as legacy</issue_title>
<issue_description>## Objective
Update
src/tnfr/operators/canonical_grammar.pyto delegate tounified_grammar.py, eliminating duplication of RC1-RC4 constraints while preserving the module as a legacy alias.Context
Per UNIFIED_GRAMMAR_RULES.md § Implementation Strategy Phase 2, the RC1-RC4 grammar system in
canonical_grammar.pyneeds to be refactored to use the unified U1-U4 constraints.Current State:
canonical_grammar.py(14KB) implements RC1-RC4 grammar rulesunified_grammar.pyimplements U1-U4 unified rulesGoal:
canonical_grammar.pyas legacy/aliasunified_grammar.pyMapping: RC1-RC4 → U1-U4
From UNIFIED_GRAMMAR_RULES.md:
Note: RNC1 (Terminators) was previously removed but restored as U1b (Closure) with proper physical basis.
Implementation Tasks
1. Add Legacy Module Warning
2. Create Legacy Aliases