-
Notifications
You must be signed in to change notification settings - Fork 6
Enhance vCon party management and documentation #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
howethomas
merged 1 commit into
main
from
54-inconsistent-mutability-of-returned-objects-vcondialog-vs-vconparties
Sep 18, 2025
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| # Mutability Consistency Fix Summary | ||
|
|
||
| ## Problem Description | ||
|
|
||
| There was an inconsistency in the mutability of objects returned by `vcon.dialog` and `vcon.parties`: | ||
|
|
||
| - **`vcon.dialog`** returned a list of dictionaries from the internal data structure, allowing users to modify dialog entries in place. | ||
| - **`vcon.parties`** returned newly constructed Party objects from the underlying data, making in-place modifications ineffective since changes were not reflected in the internal vcon_dict. | ||
|
|
||
| This inconsistency was confusing for users who expected both properties to behave the same way. | ||
|
|
||
| ## Solution Implemented | ||
|
|
||
| **Approach**: Return references to mutable objects in both cases. | ||
|
|
||
| ### Changes Made | ||
|
|
||
| 1. **Modified `vcon.parties` property** (lines 1152-1169 in `src/vcon/vcon.py`): | ||
| - Changed return type from `List[Party]` to `List[Dict[str, Any]]` | ||
| - Changed implementation from `[Party(**party) for party in self.vcon_dict.get("parties", [])]` to `self.vcon_dict.get("parties", [])` | ||
| - Updated documentation to reflect the new behavior | ||
|
|
||
| 2. **Added `get_party_objects()` method** (lines 1171-1187 in `src/vcon/vcon.py`): | ||
| - Provides backward compatibility for code that needs Party objects | ||
| - Returns `[Party(**party) for party in self.vcon_dict.get("parties", [])]` | ||
| - Includes comprehensive documentation and examples | ||
|
|
||
| 3. **Updated `vcon.dialog` property documentation** (lines 1189-1206 in `src/vcon/vcon.py`): | ||
| - Made the mutability behavior explicit in the documentation | ||
| - Added examples showing in-place modifications | ||
|
|
||
| 4. **Fixed existing tests** (lines 286-297 in `tests/test_vcon.py`): | ||
| - Updated `test_properties()` to work with the new dictionary-based approach | ||
| - Changed from `vcon.parties[0].to_dict()` to `vcon.parties[0]` | ||
|
|
||
| ## Benefits | ||
|
|
||
| 1. **Consistency**: Both `vcon.dialog` and `vcon.parties` now return mutable references | ||
| 2. **Performance**: No longer creates new objects on every access | ||
| 3. **User expectations**: Users can modify objects in place as expected | ||
| 4. **Backward compatibility**: Existing code can use `get_party_objects()` when Party objects are needed | ||
|
|
||
| ## Usage Examples | ||
|
|
||
| ### Before (Inconsistent) | ||
| ```python | ||
| # This worked - dialog was mutable | ||
| vcon.dialog[0]["body"] = "Modified content" | ||
|
|
||
| # This didn't work - parties were not mutable | ||
| vcon.parties[0].name = "Modified name" # Changes lost | ||
| ``` | ||
|
|
||
| ### After (Consistent) | ||
| ```python | ||
| # Both work - both are mutable | ||
| vcon.dialog[0]["body"] = "Modified content" | ||
| vcon.parties[0]["name"] = "Modified name" | ||
|
|
||
| # Both changes are reflected in the internal data | ||
| assert vcon.dialog[0]["body"] == "Modified content" | ||
| assert vcon.parties[0]["name"] == "Modified name" | ||
| ``` | ||
|
|
||
| ### Backward Compatibility | ||
| ```python | ||
| # For code that needs Party objects | ||
| party_objects = vcon.get_party_objects() | ||
| party_objects[0].name = "Modified name" # This creates new objects | ||
| ``` | ||
|
|
||
| ## Testing | ||
|
|
||
| - Created comprehensive tests in `tests/test_mutability_consistency.py` | ||
| - Updated existing tests to work with the new behavior | ||
| - Verified core mutability logic works correctly | ||
| - All tests pass with the new implementation | ||
|
|
||
| ## Breaking Changes | ||
|
|
||
| - **`vcon.parties`** now returns `List[Dict[str, Any]]` instead of `List[Party]` | ||
| - Code that accessed `vcon.parties[0].name` needs to be updated to `vcon.parties[0]["name"]` | ||
| - Code that needs Party objects should use `vcon.get_party_objects()` | ||
|
|
||
| ## Migration Guide | ||
|
|
||
| For existing code that uses Party objects: | ||
|
|
||
| ```python | ||
| # Old way (no longer works) | ||
| parties = vcon.parties | ||
| for party in parties: | ||
| print(party.name) | ||
|
|
||
| # New way 1: Use dictionary access | ||
| parties = vcon.parties | ||
| for party in parties: | ||
| print(party["name"]) | ||
|
|
||
| # New way 2: Use get_party_objects() for backward compatibility | ||
| party_objects = vcon.get_party_objects() | ||
| for party in party_objects: | ||
| print(party.name) | ||
| ``` | ||
|
|
||
| ## Files Modified | ||
|
|
||
| 1. `src/vcon/vcon.py` - Main implementation changes | ||
| 2. `tests/test_vcon.py` - Updated existing tests | ||
| 3. `tests/test_mutability_consistency.py` - New comprehensive tests | ||
| 4. `MUTABILITY_FIX_SUMMARY.md` - This documentation | ||
|
|
||
| The fix successfully resolves the mutability inconsistency while maintaining backward compatibility through the new `get_party_objects()` method. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| """ | ||
| Test to demonstrate and verify the mutability consistency between vcon.dialog and vcon.parties properties. | ||
| """ | ||
| import pytest | ||
| from vcon import Vcon | ||
| from vcon.party import Party | ||
| from vcon.dialog import Dialog | ||
| from datetime import datetime, timezone | ||
|
|
||
|
|
||
| def test_mutability_consistency_demonstration(): | ||
| """ | ||
| Demonstrate that both vcon.dialog and vcon.parties now work consistently. | ||
|
|
||
| This test shows that: | ||
| - vcon.dialog returns mutable references (in-place modifications work) | ||
| - vcon.parties now also returns mutable references (in-place modifications work) | ||
| """ | ||
| # Create a vCon with parties and dialogs | ||
| vcon = Vcon.build_new() | ||
|
|
||
| # Add parties | ||
| party1 = Party(name="Alice", tel="+1234567890") | ||
| party2 = Party(name="Bob", tel="+1987654321") | ||
| vcon.add_party(party1) | ||
| vcon.add_party(party2) | ||
|
|
||
| # Add dialogs | ||
| dialog1 = Dialog( | ||
| type="text", | ||
| start=datetime.now(timezone.utc).isoformat(), | ||
| parties=[0, 1], | ||
| body="Hello, this is a test message" | ||
| ) | ||
| dialog2 = Dialog( | ||
| type="text", | ||
howethomas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| start=datetime.now(timezone.utc).isoformat(), | ||
| parties=[0], | ||
| body="This is another message" | ||
| ) | ||
| vcon.add_dialog(dialog1) | ||
| vcon.add_dialog(dialog2) | ||
|
|
||
| # Test dialog mutability (should work - returns direct references) | ||
| dialog_list = vcon.dialog | ||
| original_body = dialog_list[0]["body"] | ||
| dialog_list[0]["body"] = "Modified dialog body" | ||
|
|
||
| # Verify the change is reflected in the internal data | ||
| assert vcon.dialog[0]["body"] == "Modified dialog body" | ||
| assert vcon.vcon_dict["dialog"][0]["body"] == "Modified dialog body" | ||
|
|
||
| # Test parties mutability (now works - returns mutable references) | ||
| parties_list = vcon.parties | ||
| original_name = parties_list[0]["name"] | ||
|
|
||
| # This modification now affects the internal data | ||
| parties_list[0]["name"] = "Modified Alice" | ||
|
|
||
| # Verify the change IS reflected in the internal data (new consistent behavior) | ||
| assert vcon.parties[0]["name"] == "Modified Alice" # Modified name | ||
| assert vcon.vcon_dict["parties"][0]["name"] == "Modified Alice" # Internal data changed | ||
|
|
||
|
|
||
| def test_mutability_consistency_after_fix(): | ||
| """ | ||
| Test that after the fix, both vcon.dialog and vcon.parties behave consistently. | ||
|
|
||
| This test verifies that both properties return mutable references. | ||
| """ | ||
| # Create a vCon with parties and dialogs | ||
| vcon = Vcon.build_new() | ||
|
|
||
| # Add parties | ||
| party1 = Party(name="Alice", tel="+1234567890") | ||
| party2 = Party(name="Bob", tel="+1987654321") | ||
| vcon.add_party(party1) | ||
| vcon.add_party(party2) | ||
|
|
||
| # Add dialogs | ||
| dialog1 = Dialog( | ||
| type="text", | ||
| start=datetime.now(timezone.utc).isoformat(), | ||
| parties=[0, 1], | ||
| body="Hello, this is a test message" | ||
| ) | ||
| vcon.add_dialog(dialog1) | ||
|
|
||
| # Test dialog mutability (should work) | ||
| dialog_list = vcon.dialog | ||
| dialog_list[0]["body"] = "Modified dialog body" | ||
| assert vcon.dialog[0]["body"] == "Modified dialog body" | ||
| assert vcon.vcon_dict["dialog"][0]["body"] == "Modified dialog body" | ||
|
|
||
| # Test parties mutability (should work after fix) | ||
| parties_list = vcon.parties | ||
| parties_list[0]["name"] = "Modified Alice" | ||
|
|
||
| # Verify the change IS reflected in the internal data (after fix) | ||
| assert vcon.parties[0]["name"] == "Modified Alice" | ||
| assert vcon.vcon_dict["parties"][0]["name"] == "Modified Alice" | ||
|
|
||
| # Test that we can still access party data as dictionaries | ||
| assert isinstance(vcon.parties[0], dict) | ||
| assert "name" in vcon.parties[0] | ||
| assert "tel" in vcon.parties[0] | ||
|
|
||
|
|
||
| def test_party_object_creation_still_works(): | ||
| """ | ||
| Test that we can still create Party objects from the dictionary data when needed. | ||
| """ | ||
| vcon = Vcon.build_new() | ||
|
|
||
| # Add a party | ||
| party = Party(name="Alice", tel="+1234567890") | ||
| vcon.add_party(party) | ||
|
|
||
| # Get the party data as dictionary | ||
| party_dict = vcon.parties[0] | ||
|
|
||
| # Create a Party object from the dictionary data | ||
| party_obj = Party(**party_dict) | ||
|
|
||
| # Verify the Party object works correctly | ||
| assert party_obj.name == "Alice" | ||
| assert party_obj.tel == "+1234567890" | ||
| assert isinstance(party_obj, Party) | ||
|
|
||
|
|
||
| def test_backward_compatibility(): | ||
| """ | ||
| Test that existing code that expects Party objects still works. | ||
| """ | ||
| vcon = Vcon.build_new() | ||
|
|
||
| # Add a party | ||
| party = Party(name="Alice", tel="+1234567890") | ||
| vcon.add_party(party) | ||
|
|
||
| # Get parties and create Party objects (common pattern) | ||
| parties = [Party(**party_dict) for party_dict in vcon.parties] | ||
|
|
||
| # Verify the Party objects work correctly | ||
| assert len(parties) == 1 | ||
| assert parties[0].name == "Alice" | ||
| assert parties[0].tel == "+1234567890" | ||
| assert isinstance(parties[0], Party) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.