Addressed all claude analysis recommendations of #1240

[VisLab]

No description provided.

[Copilot]

Pull request overview

This PR tightens error semantics and improves developer-facing diagnostics across HED query parsing, group copying, and definition resolution, while also cleaning up debug scaffolding and expanding module/serializer documentation.

Changes:

• Introduces HedQueryError (a ValueError subclass) and updates query parsing to raise it consistently, with new tests to lock in behavior.
• Adjusts HedGroup shallow-copy behavior to raise copy.Error and adds copy-focused tests.
• Improves docstrings/error messages and removes leftover __main__ debug blocks in validator utilities.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/models/test_query_handler.py	Adds coverage ensuring broken queries raise HedQueryError (and remain catchable as ValueError).
tests/models/test_hed_group.py	Adds tests for shallow-copy failure (copy.Error) and successful deep-copy behavior.
hed/validator/util/char_util.py	Removes __main__ debug snippet.
hed/validator/reserved_checker.py	Removes __main__ debug snippet.
hed/schema/schema_io/schema2base.py	Substantially expands serializer base-class documentation and clarifies traversal order.
hed/schema/hed_schema_entry.py	Avoids duplicated inherited-attribute checks by computing once and reusing the value.
hed/schema/hed_schema.py	Removes commented-out debug prints from equality logic.
hed/schema/init.py	Adds richer package docstring explaining intended public API/usage.
hed/models/query_handler.py	Switches parse errors from ValueError to HedQueryError.
hed/models/hed_string.py	Documents the intentional fallback-to-empty-tree behavior on structural parse failures.
hed/models/hed_group.py	Changes shallow-copy exception type/message to copy.Error with guidance to use .copy().
hed/models/def_expand_gather.py	Replaces generic “Invalid Definition” with more actionable ValueError messages.
hed/models/column_mapper.py	Moves partial import to module scope and removes redundant inline imports/branches.
hed/models/init.py	Adds richer package docstring explaining intended public API/usage.
hed/errors/exceptions.py	Adds the new HedQueryError exception type.
hed/init.py	Re-exports HedQueryError at package top-level.

---

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

[github-actions]

PR Review: #1246 — Addressed Claude analysis recommendations of #1240

Overall: Clean, well-structured PR. No critical issues. The changes fall into four categories: better exception granularity, documentation improvements, dead code cleanup, and a minor refactoring.

---

HedQueryError (exceptions.py / query_handler.py)

Well designed. Inheriting from ValueError preserves backward compatibility while enabling finer-grained except HedQueryError handling. All raise sites in query_handler.py are updated consistently. The two new tests correctly verify both the specific type and the ValueError supertype.

---

copy.Error in HedGroup.copy (hed_group.py)

copy is already imported; copy.Error is a valid Python exception. Using it is more semantically appropriate than ValueError for a copy-operation failure. Test added.

---

_finalize_inherited_attributes refactoring (hed_schema_entry.py)

The old code called _check_inherited_attribute twice per attribute (once for the bool guard, once for the value). The new code calls it once with return_value=True and guards on is not None.

This is safe because _check_inherited_attribute returns None only when attribute_values is empty (no attribute found in the hierarchy), and schema attribute values are never stored as None — the codebase treats None as the "not present" sentinel consistently (see has_attribute). The refactoring is correct and eliminates the redundant call.

---

Documentation

• hed/models/__init__.py and hed/schema/__init__.py docstrings: all listed exports are verified to exist in the actual from .x import y statements. ✓
• schema2base.py class docstring is accurate.
• Inline comment added to hed_string.py explaining the silent ValueError catch is appropriate.

---

Cleanup

• Removing if __name__ == "__main__" blocks from reserved_checker.py and char_util.py is correct — these were ad-hoc manual test blocks.
• Removing commented-out debug print statements from hed_schema.py is correct.
• Moving from functools import partial to module level in column_mapper.py follows PEP 8; the no-op else: assign_to_column = assign_to_column removal is correct.

---

Minor issues (inline)

See inline comments below.

[github-actions]

PR Review: #1246 — Addressed Claude analysis recommendations of #1240

Overall: Clean, well-structured PR. No critical issues. The changes fall into four categories: better exception granularity, documentation improvements, dead code cleanup, and a minor refactoring.

---

HedQueryError (exceptions.py / query_handler.py)

Well designed. Inheriting from ValueError preserves backward compatibility while enabling finer-grained except HedQueryError handling. All raise sites in query_handler.py are updated consistently. The two new tests correctly verify both the specific type and the ValueError supertype.

---

copy.Error in HedGroup.copy (hed_group.py)

copy is already imported; copy.Error is a valid Python exception. Using it is more semantically appropriate than ValueError for a copy-operation failure. Test added.

One BC note: copy.Error inherits from Exception, not ValueError. Any downstream code catching ValueError from copy.copy(hed_group) would now get an uncaught copy.Error. Practical risk is very low (this is always an error path), but worth a mention in release notes.

---

_finalize_inherited_attributes refactoring (hed_schema_entry.py)

The old code called _check_inherited_attribute twice per attribute (once for the bool guard, once for the value). The new code calls it once with return_value=True and guards on is not None.

This is safe because _check_inherited_attribute returns None only when attribute_values is empty (no attribute found in the hierarchy), and schema attribute values are never stored as None — the codebase treats None as the "not present" sentinel consistently (see has_attribute). The refactoring is correct and eliminates the redundant call.

---

_output_extras now called in process_schema (schema2base.py)

This PR adds self._output_extras(hed_schema) to process_schema for the first time, activating previously dead code in schema2json.py (_output_sources, _output_prefixes, _output_external_annotations). All three methods are guarded by if ... is None or ... empty: return, so for standard schemas without these extras the behavior is unchanged. For schemas that carry sources/prefixes/external-annotation data, JSON output will now include these sections — which is the correct behavior.

---

Documentation

• hed/models/__init__.py and hed/schema/__init__.py docstrings: all listed exports are verified to exist in the actual from .x import y statements. ✓
• schema2base.py class docstring is accurate.
• Inline comment added to hed_string.py explaining the silent ValueError catch is appropriate.

---

Cleanup

• Removing if __name__ == "__main__" blocks from reserved_checker.py and char_util.py is correct — these were ad-hoc manual test blocks.
• Removing commented-out debug print statements from hed_schema.py is correct.
• Moving from functools import partial to module level in column_mapper.py follows PEP 8; the no-op else: assign_to_column = assign_to_column removal is correct.

---

Minor issues (inline)

See inline comments below.

[github-actions]

Suggestion — duplicate coverage

test_broken_search_strings_raise_hed_query_error tests exactly the same six broken strings as the existing test_broken_search_strings (lines 41-46). Since HedQueryError IS a ValueError, the old test already implicitly passes for the new type. Consider either:

• Replacing the old test's assertRaises(ValueError) with assertRaises(HedQueryError) (which covers both, since HedQueryError is a ValueError), or
• Adding a comment explaining why the duplication is intentional (e.g., to guard against accidentally reverting the type to plain ValueError).

[github-actions]

Suggestion — _parent state is not verified

copy.deepcopy(group) is called while group._parent still points to hed_string. Python's cycle-detection handles this correctly, but the resulting deep object will have _parent set to a deep copy of the entire HedString tree — which is different from the documented group.copy() behaviour (where _parent is None in the returned copy).

The test only checks str(deep) == str(group), which passes regardless. If the intent is to document that raw deepcopy is safe, add an assertion (or at least a comment) noting that deep._parent will be a copy of the parent, not None. If the intent is to test the public deep-copy API, group.copy() (already tested on line 137) is the right path.