Tighten restrictions and errors for tag APIs - #3
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes tag lookup APIs stricter across GBL3/GBL4/EBL images by adding an allow_missing keyword argument to get_tags() and defaulting it to False, so missing tags raise instead of silently returning []/None. Tests are updated to opt into missing-tag behavior where appropriate.
Changes:
- Add
allow_missing: bool = Falsetoget_tags()in GBL3/GBL4/EBL and raiseKeyErrorby default when no tags are found. - Update tests and internal call sites to pass
allow_missing=Truewhere “no tags” is expected/acceptable. - Remove
find_first_tag()/has_tag()helpers and simplifyget_first_tag()implementations to index intoget_tags()results.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_rare_tags.py | Updates a missing-tag assertion to use get_tags(..., allow_missing=True) returning []. |
| tests/test_gbl4.py | Updates unknown-tag checks and padding iteration to opt into missing-tag behavior. |
| tests/test_gbl.py | Updates lookup helper expectations and adds coverage for strict KeyError behavior on missing tags (GBL3). |
| tests/test_elf.py | Updates bootloader/metadata presence checks to use allow_missing=True. |
| tests/test_crypto.py | Updates signature absence assertion to use get_tags(..., allow_missing=True) returning []. |
| tests/test_compression.py | Updates “tag removed” assertions to opt into missing-tag behavior. |
| tests/test_commander_cli.py | Updates “skip if no LZMA tags” logic and list comprehensions to use allow_missing=True where appropriate. |
| pygbl/gbl4.py | Changes GBL4Image.get_tags() to be strict by default and removes find_first_tag()/has_tag(). |
| pygbl/gbl3.py | Changes GBL3Image.get_tags() to be strict by default and updates internal helpers to use allow_missing=True where needed. |
| pygbl/ebl.py | Changes EBLImage.get_tags() to be strict by default and removes find_first_tag()/has_tag(). |
Suppressed comments (1)
pygbl/ebl.py:295
EBLImage.get_tags()now has a new strict-by-default behavior and anallow_missingopt-in, but there are no tests exercising either theKeyErrordefault or theallow_missing=Truepath for EBL images. Adding a small unit test intests/test_ebl.pywould help prevent regressions in this API change.
def get_tags(self, tag_type: type[T], *, allow_missing: bool = False) -> list[T]:
tags: list[T] = [t for t in self.tags if type(t) is tag_type]
if not tags and not allow_missing:
raise KeyError(f"No {tag_type.__name__} tag exists")
return tags
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+732
to
+741
| def get_tags(self, tag_type: type[T], *, allow_missing: bool = False) -> list[T]: | ||
| tags: list[T] = [t for t in self.tags if type(t) is tag_type] | ||
|
|
||
| def find_first_tag(self, tag_type: type[T]) -> T | None: | ||
| return next((t for t in self.tags if type(t) is tag_type), None) | ||
|
|
||
| def get_first_tag(self, tag_type: type[T]) -> T: | ||
| tag = self.find_first_tag(tag_type) | ||
|
|
||
| if tag is None: | ||
| if not tags and not allow_missing: | ||
| raise KeyError(f"No {tag_type.__name__} tag exists") | ||
|
|
||
| return tag | ||
| return tags | ||
|
|
||
| def has_tag(self, tag_type: type[Any]) -> bool: | ||
| return self.find_first_tag(tag_type) is not None | ||
| def get_first_tag(self, tag_type: type[T]) -> T: | ||
| return self.get_tags(tag_type)[0] |
Comment on lines
+289
to
+298
| def get_tags(self, tag_type: type[T], *, allow_missing: bool = False) -> list[T]: | ||
| tags: list[T] = [t for t in self.tags if type(t) is tag_type] | ||
|
|
||
| def find_first_tag(self, tag_type: type[T]) -> T | None: | ||
| return next((t for t in self.tags if type(t) is tag_type), None) | ||
|
|
||
| def get_first_tag(self, tag_type: type[T]) -> T: | ||
| tag = self.find_first_tag(tag_type) | ||
|
|
||
| if tag is None: | ||
| if not tags and not allow_missing: | ||
| raise KeyError(f"No {tag_type.__name__} tag exists") | ||
|
|
||
| return tag | ||
| return tags | ||
|
|
||
| def has_tag(self, tag_type: type[Any]) -> bool: | ||
| return self.find_first_tag(tag_type) is not None | ||
| def get_first_tag(self, tag_type: type[T]) -> T: | ||
| return self.get_tags(tag_type)[0] |
Comment on lines
537
to
+557
| @@ -548,20 +548,10 @@ def walk(tags: list[GBL4TagBase]) -> None: | |||
|
|
|||
| walk([self.root]) | |||
|
|
|||
| return found | |||
|
|
|||
| def find_first_tag(self, tag_type: type[T]) -> T | None: | |||
| tags = self.get_tags(tag_type) | |||
|
|
|||
| return tags[0] if tags else None | |||
|
|
|||
| def get_first_tag(self, tag_type: type[T]) -> T: | |||
| tag = self.find_first_tag(tag_type) | |||
|
|
|||
| if tag is None: | |||
| if not found and not allow_missing: | |||
| raise KeyError(f"No {tag_type.__name__} tag exists") | |||
|
|
|||
| return tag | |||
| return found | |||
|
|
|||
| def has_tag(self, tag_type: type[Any]) -> bool: | |||
| return self.find_first_tag(tag_type) is not None | |||
| def get_first_tag(self, tag_type: type[T]) -> T: | |||
| return self.get_tags(tag_type)[0] | |||
Comment on lines
64
to
+66
| def test_no_unknown_tags(path: pathlib.Path) -> None: | ||
| """Every tag in a real image should be one we model.""" | ||
| assert load(path).get_tags(GBL4UnknownTag) == [] | ||
| assert load(path).get_tags(GBL4UnknownTag, allow_missing=True) == [] |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add an
allow_missingkwarg and set it toFalseby default. ReturningNoneor an empty list of tags is something to opt into, as most use cases should crash when a tag is missing.