pythonic getter implementation by keighrim · Pull Request #335 · clamsproject/mmif-python

keighrim · 2025-11-18T21:54:16Z

fixes #295. See the issue for discussion.

…e superclass)

… ... * added `get_by_key()` for string queries * `__getitem__()` now only works with integer/slice indexing

…ist-like classes' `get` once and for all

Copilot

Pull Request Overview

This PR implements pythonic getter patterns for MMIF objects, addressing issue #295. The changes separate list-like positional access from dictionary-like ID-based access, making the API more intuitive and consistent with Python conventions.

Key Changes

DataList collections (views, documents, annotations) now only support integer/slice indexing (list-like behavior)
High-level containers (Mmif, View) provide string-based ID access via bracket notation
Added get() method to MmifObject base class for safe access with default values across all MMIF objects
Deprecated the get() method on DataList with a clear migration path

Reviewed Changes

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

Show a summary per file

File	Description
tests/test_serialize.py	Updated tests to use new access patterns; added comprehensive test coverage for integer indexing, slicing, and string access behavior; updated error handling test
tests/mmif_examples.py	Added helper function to load test data from local git repository or URL for offline development
mmif/serialize/model.py	Refactored DataList.getitem() to only accept int/slice; deprecated DataList.get(); added MmifObject.get() for safe access; enhanced documentation
mmif/serialize/view.py	Updated View.getitem() to use _items dict directly; added comprehensive documentation with examples
mmif/serialize/mmif.py	Refactored Mmif.getitem() for clearer error handling and to use _items dict directly; enhanced documentation
mmif/serialize/annotation.py	Enhanced documentation for get() methods in Annotation and Document classes; fixed default parameter forwarding in Document.get()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mmif/serialize/mmif.py

Copilot · 2025-11-18T22:22:37Z

tests/mmif_examples.py

+                    result = subprocess.run(
+                        ['git', 'show', git_ref],
+                        cwd=localmmif,
+                        capture_output=True,
+                        text=True,
+                        check=True
+                    )


Using subprocess.run() with shell=False is good for security, but the cwd parameter is set from an environment variable. Consider validating that localmmif is a valid directory path to prevent potential security issues:

if localmmif: if not os.path.isdir(localmmif): raise ValueError(f"LOCALMMIF path is not a valid directory: {localmmif}") # ... rest of code

This prevents potential issues if the environment variable is set to an invalid or malicious path.

@copilot open a new pull request to apply changes based on this feedback Use pathlib, not os.path

Copilot · 2025-11-18T22:22:37Z

tests/test_serialize.py

        err_str = 'custom error as a single long string'
-        aview.metadata.error = err_str
+        aview.metadata.error = ErrorDict({'message': err_str})
+        print(aview.metadata.error)


Debug print statement should be removed before merging. This appears to be leftover debugging code that should not be in the final test suite.

Suggested change

print(aview.metadata.error)

tests/test_serialize.py

Copilot · 2025-11-18T22:22:38Z

mmif/serialize/model.py

            return False

-    def __getitem__(self, key) -> Union['MmifObject', str, datetime]:
+    def __getitem__(self, key) -> Any:


This method raises AttributeError - should raise a LookupError (KeyError or IndexError) instead.

@copilot open a new pull request to apply changes based on this feedback, Implement KeyError

keighrim

go fix them. Also check the failed test at https://github.com/clamsproject/mmif-python/actions/runs/19481626098

mmif/serialize/mmif.py

keighrim · 2025-11-18T23:12:37Z

mmif/serialize/model.py

            return False

-    def __getitem__(self, key) -> Union['MmifObject', str, datetime]:
+    def __getitem__(self, key) -> Any:


@copilot open a new pull request to apply changes based on this feedback, Implement KeyError

keighrim · 2025-11-18T23:13:54Z

tests/mmif_examples.py

+                    result = subprocess.run(
+                        ['git', 'show', git_ref],
+                        cwd=localmmif,
+                        capture_output=True,
+                        text=True,
+                        check=True
+                    )


@copilot open a new pull request to apply changes based on this feedback Use pathlib, not os.path

Copilot · 2025-11-18T23:16:34Z

@keighrim I've opened a new pull request, #336, to work on those changes. Once the pull request is ready, I'll request review from you.

…dation Co-authored-by: keighrim <9062727+keighrim@users.noreply.github.com>

[WIP] Update address feedback on Pythonic getter implementation

codecov · 2025-11-19T00:41:54Z

Codecov Report

❌ Patch coverage is 92.10526% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.19%. Comparing base (b08fd6e) to head (e32ec86).
⚠️ Report is 10 commits behind head on develop.

Files with missing lines	Patch %	Lines
mmif/serialize/mmif.py	94.73%	1 Missing ⚠️
mmif/serialize/model.py	93.75%	1 Missing ⚠️
mmif/serialize/view.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #335      +/-   ##
===========================================
+ Coverage    87.98%   88.19%   +0.20%     
===========================================
  Files           13       13              
  Lines         1673     1677       +4     
===========================================
+ Hits          1472     1479       +7     
+ Misses         201      198       -3

Flag	Coverage Δ
unittests	`88.19% <92.10%> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

keighrim added 5 commits August 9, 2025 20:35

promoted get() with default value from Mmif to MmifObject (prim…

c2ba4c3

…e superclass)

deprecated get() from list-like objs, separate key-index, int-index…

6f95b04

… ... * added `get_by_key()` for string queries * `__getitem__()` now only works with integer/slice indexing

big updates on docstring based on current get_by_id impl

c5ae73a

Merge branch 'develop' into 295-pythonic-getters

dcf7a4f

get rid of intermediate get_by_key implementation and deprecating l…

15dd3df

…ist-like classes' `get` once and for all

clams-bot added this to infra Nov 18, 2025

github-project-automation bot moved this to Todo in infra Nov 18, 2025

keighrim requested a review from Copilot November 18, 2025 22:18

Copilot started reviewing on behalf of keighrim November 18, 2025 22:19 View session

Copilot finished reviewing on behalf of keighrim November 18, 2025 22:21

Copilot AI reviewed Nov 18, 2025

View reviewed changes

keighrim commented Nov 18, 2025

View reviewed changes

Initial plan

d0024a6

Copilot AI mentioned this pull request Nov 18, 2025

[WIP] Update address feedback on Pythonic getter implementation #336

Merged

4 tasks

Copilot AI and others added 3 commits November 18, 2025 23:21

Address PR feedback: simplify returns, use KeyError, add pathlib vali…

ad8d4e2

…dation Co-authored-by: keighrim <9062727+keighrim@users.noreply.github.com>

Merge pull request #336 from clamsproject/copilot/sub-pr-335

72c5e82

[WIP] Update address feedback on Pythonic getter implementation

fixed syntax error in docstrings

e32ec86

keighrim merged commit 9c0d3ba into develop Nov 19, 2025
6 checks passed

keighrim deleted the 295-pythonic-getters branch November 19, 2025 01:28

github-project-automation bot moved this from Todo to Done in infra Nov 19, 2025

Conversation

keighrim commented Nov 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

keighrim Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

keighrim Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

keighrim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keighrim Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

keighrim Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Nov 18, 2025

Uh oh!

codecov bot commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Nov 19, 2025 •

edited

Loading