-
Notifications
You must be signed in to change notification settings - Fork 12
Addressed review in #1247 #1263
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -63,14 +63,14 @@ def __init__(self, filename, schema_as_string=None, schema=None, file_format=Non | |||||||||
| self.appending_to_schema = True | ||||||||||
| if not self._schema.with_standard: | ||||||||||
| raise HedFileError( | ||||||||||
| HedExceptions.SCHEMA_DUPLICATE_PREFIX, | ||||||||||
| HedExceptions.SCHEMA_LOAD_FAILED, | ||||||||||
| "Loading multiple normal schemas as a merged one with the same namespace. " | ||||||||||
| "Ensure schemas have the withStandard header attribute set", | ||||||||||
| self.name, | ||||||||||
| ) | ||||||||||
| elif with_standard != self._schema.with_standard: | ||||||||||
| raise HedFileError( | ||||||||||
| HedExceptions.BAD_WITH_STANDARD_MULTIPLE_VALUES, | ||||||||||
| HedExceptions.SCHEMA_LOAD_FAILED, | ||||||||||
| f"Merging schemas requires same withStandard value ({with_standard} != {self._schema.with_standard}).", | ||||||||||
| self.name, | ||||||||||
| ) | ||||||||||
|
|
@@ -125,7 +125,7 @@ def _load(self): | |||||||||
| base_version = load_schema_version(self._schema.with_standard, check_prerelease=self.check_prerelease) | ||||||||||
| except HedFileError as e: | ||||||||||
| raise HedFileError( | ||||||||||
| HedExceptions.BAD_WITH_STANDARD, | ||||||||||
| HedExceptions.SCHEMA_LIBRARY_INVALID, | ||||||||||
| message=f"Cannot load withStandard schema '{self._schema.with_standard}'", | ||||||||||
| filename=e.filename, | ||||||||||
| ) from e | ||||||||||
|
|
@@ -194,36 +194,36 @@ def find_rooted_entry(tag_entry, schema, loading_merged): | |||||||||
| if rooted_tag is not None: | ||||||||||
| if not schema.with_standard: | ||||||||||
| raise HedFileError( | ||||||||||
| HedExceptions.ROOTED_TAG_INVALID, | ||||||||||
| HedExceptions.SCHEMA_ATTRIBUTE_INVALID, | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Either:
Suggested change
|
||||||||||
| HedExceptions.SCHEMA_ATTRIBUTE_INVALID, | |
| HedExceptions.SCHEMA_LIBRARY_INVALID, |
Copilot
AI
Mar 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message string ends with an extra escaped quote (... is not a string."), which will produce a confusing message (trailing "). Remove the stray quote so the message is clean.
| f"Rooted tag '{tag_entry.short_tag_name}' is not a string.\"", | |
| f"Rooted tag '{tag_entry.short_tag_name}' is not a string.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: AttributeError at runtime — HedExceptions.LIBRARY_SCHEMA_INVALID does not exist. The constant is SCHEMA_LIBRARY_INVALID (note the word order).
| HedExceptions.LIBRARY_SCHEMA_INVALID, | |
| HedExceptions.SCHEMA_LIBRARY_INVALID, |
Copilot
AI
Mar 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HedExceptions.LIBRARY_SCHEMA_INVALID is referenced here, but HedExceptions (hed/errors/exceptions.py) does not define this constant. This will raise an AttributeError at runtime. Use an existing error code (e.g., SCHEMA_LIBRARY_INVALID if appropriate) or define LIBRARY_SCHEMA_INVALID in HedExceptions.
| HedExceptions.LIBRARY_SCHEMA_INVALID, | |
| HedExceptions.SCHEMA_LIBRARY_INVALID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Incomplete removal —
AttributeErrorat runtime in three filesIN_LIBRARY_IN_UNMERGEDandINVALID_LIBRARY_PREFIXwere removed fromHedExceptions, but they are still referenced in files not touched by this PR:hed/schema/schema_io/xml2schema.pyHedExceptions.IN_LIBRARY_IN_UNMERGEDhed/schema/schema_io/df2schema.pyHedExceptions.IN_LIBRARY_IN_UNMERGEDhed/schema/schema_io/wiki2schema.pyHedExceptions.IN_LIBRARY_IN_UNMERGEDhed/schema/hed_schema.pyHedExceptions.INVALID_LIBRARY_PREFIXEach of those call sites needs to be updated to its replacement code (
SCHEMA_LIBRARY_INVALIDforIN_LIBRARY_IN_UNMERGED, andSCHEMA_LIBRARY_INVALIDforINVALID_LIBRARY_PREFIX) before merging.