Addressed claude interface eval #1264

[VisLab]

No description provided.

[Copilot]

Pull request overview

This PR updates the HED Python codebase to improve interface consistency by standardizing constants/enums (notably error codes and find_top_level_tags return selection), updating call sites/tests accordingly, and expanding/adjusting package-level exports and documentation.

Changes:

• Introduce TopTagReturnType and replace numeric include_groups usage with the enum for clearer HedString.find_top_level_tags calls.
• Standardize several exception/error-code string literals to uppercase-style identifiers and update tests accordingly.
• Refactor public exports in multiple __init__.py modules and expand docstrings for schema/model classes.

Reviewed changes

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

Show a summary per file

File	Description
tests/tools/analysis/test_temporal_event.py	Updates tests to use TopTagReturnType instead of numeric include_groups.
tests/schema/test_schema_validation_util.py	Updates tests to use renamed schema validation util functions.
tests/models/test_sidecar.py	Updates imports and asserts to use HedExceptions.FILE_NOT_FOUND and direct DefinitionEntry import.
tests/models/test_column_mapper.py	Updates ColumnMapper import path to match new exports.
spec_tests/test_hed_cache.py	Updates expected exception code to HedExceptions.FILE_NOT_FOUND.
hed/validator/spreadsheet_validator.py	Uses TopTagReturnType.TAGS for find_top_level_tags return selection.
hed/validator/hed_validator.py	Updates imports and uses TopTagReturnType.GROUPS for definition-group extraction.
hed/tools/util/init.py	Adds re-exports of common util helpers from data_util/io_util.
hed/tools/bids/init.py	Narrows re-exports from bids_util to only parse_bids_filename.
hed/tools/analysis/event_manager.py	Uses TopTagReturnType.BOTH for temporal-event extraction.
hed/schema/schema_validation/validation_util.py	Renames *_new validators to stable names.
hed/schema/schema_validation/compliance.py	Updates imports/calls to renamed schema validation functions.
hed/schema/schema_validation/init.py	Updates re-export list to renamed validation functions.
hed/schema/hed_schema_section.py	Expands class docstring for schema section container.
hed/schema/hed_schema_entry.py	Expands docstrings for schema entry classes.
hed/schema/init.py	Adjusts schema package exports (removes some re-exports).
hed/models/model_constants.py	Adds TopTagReturnType enum for find_top_level_tags.
hed/models/hed_string.py	Updates find_top_level_tags docs and uses TopTagReturnType in internal calls.
hed/models/definition_entry.py	Expands DefinitionEntry docstring.
hed/models/column_mapper.py	Expands ColumnMapper docstring and minor wording tweaks.
hed/models/init.py	Adjusts model package exports (adds DefTagNames/TopTagReturnType, removes some re-exports).
hed/errors/exceptions.py	Standardizes several HedExceptions string values to uppercase identifiers.
hed/errors/error_types.py	Converts ErrorSeverity to IntEnum and standardizes multiple error-code string values.
hed/errors/init.py	Expands error-related re-exports.
hed/init.py	Adjusts top-level exports (adds HedGroup, removes BaseInput).

Comments suppressed due to low confidence (2)

hed/init.py:9

• Removing BaseInput from the top-level hed exports can break downstream imports (from hed import BaseInput). If this is intended, it should be treated as a breaking change with a migration path (e.g., keep a deprecated re-export for at least one release) to avoid surprising runtime ImportErrors for library users.

from hed.models.hed_string import HedString
from hed.models.hed_tag import HedTag
from hed.models.hed_group import HedGroup
from hed.errors.error_reporter import get_printable_issue_string
from hed.errors.exceptions import HedFileError, HedExceptions, HedQueryError
from hed.models.spreadsheet_input import SpreadsheetInput
from hed.models.tabular_input import TabularInput
from hed.models.sidecar import Sidecar
from hed.models.definition_dict import DefinitionDict

hed/tools/bids/init.py:9

• This init no longer re-exports several bids_util helpers (walk_back, get_candidates, matches_criteria). If users previously relied on from hed.tools.bids import walk_back (etc.), this becomes a breaking change. Consider either keeping deprecated re-exports here or documenting the new import path (hed.tools.bids.bids_util).

from .bids_sidecar_file import BidsSidecarFile
from .bids_tabular_file import BidsTabularFile
from .bids_util import parse_bids_filename

---

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

You can also share your feedback on Copilot code review. Take the survey.

[github-actions]

Review: PR #1265 — Claude Interface Eval Cleanup

Summary

This PR brings valuable improvements: the new TopTagReturnType enum replaces magic ints, docstrings are much improved, and the _new-suffix function renames are long overdue. However, several changes constitute unannounced breaking API changes that will affect downstream users and tooling that serialises or inspects HED error codes.

Critical: Error-code strings changed from camelCase to UPPER_CASE

At least 16 error-code string values were silently renamed (e.g. "fileNotFound" -> "FILE_NOT_FOUND", "invalidTagCharacter" -> "INVALID_TAG_CHARACTER", "invalidCaps" -> "SCHEMA_INVALID_CAPITALIZATION", "blankValueString" -> "BLANK_HED_STRING", "wrongNumberPlaceholderTags" -> "WRONG_NUMBER_PLACEHOLDER_TAGS"). Affected classes: HedExceptions, ValidationErrors, SidecarErrors, SchemaWarnings, DefinitionErrors.

Any code that compares issue["code"] or exception.args[0] against the old string literals will silently stop matching. This includes external tools, persistent logs/reports, and any spec-test suite that loads error codes from JSON (once the hed-tests submodule is populated). Two tests were updated in this PR, but the rename is a public-API contract change that needs a changelog entry and a version bump signal.

Important: Public re-exports removed without deprecation

The following names were accessible via their package-level init.py and are now unexported (though the underlying modules still exist):

Removed from	Name(s)
hed	BaseInput
hed.models	ColumnMapper, DefinitionEntry
hed.schema	HedSchemaEntry, UnitClassEntry, UnitEntry, HedTagEntry, HedSchemaSection
hed.tools.bids	walk_back, get_candidates, matches_criteria

External code that imports these names from their documented locations will get an ImportError. At minimum, this needs a deprecation notice (re-export with a DeprecationWarning) or a clear breaking-change note in the release. The test files (test_column_mapper.py, test_sidecar.py) were updated to import from the concrete module paths — that is a fine workaround for tests but it signals the break rather than fixing it.

Suggestions

• TopTagReturnType is a net improvement; consider noting that raw int values (0/1/2) still work via IntEnum equality.
• The ErrorSeverity(IntEnum) change is backward-compatible (int comparisons on issue dicts continue to work).

[github-actions]

Critical: ColumnMapper and DefinitionEntry removed from hed.models public API without deprecation

Both were previously importable as from hed.models import ColumnMapper / from hed.models import DefinitionEntry. The test files themselves had to be updated to use the private-module paths, confirming these are real regressions for published users.

If this is a deliberate 1.0.0 cleanup, add deprecation re-exports in this release and document in CHANGELOG.md. Otherwise restore the imports.

[github-actions]

Important: walk_back, get_candidates, and matches_criteria dropped from hed.tools.bids exports without deprecation

All three functions remain in bids_util.py (tests still import them directly from the module), but they are no longer accessible via the package-level from hed.tools.bids import get_candidates. Any external code relying on the package-level import will break.

If these are considered internal helpers, document that decision; otherwise restore the exports or provide a one-release deprecation path.

[github-actions]

Code Review Summary

This PR makes several welcome improvements — the TopTagReturnType enum, the _new-suffix rename, the draft-PR CI skip, and the expanded docstrings are all solid. However, there are 4 critical and 1 important backward-compatibility issues that must be resolved before merging.

---

Critical

#	Location	Issue
1	hed/errors/error_types.py, hed/errors/exceptions.py	Error code string values changed. Constants like INVALID_TAG_CHARACTER, BLANK_HED_STRING, FILE_NOT_FOUND, etc. previously held camelCase strings ("invalidTagCharacter", "blankValueString", "fileNotFound"). These strings appear in the "code" key of every issue dict returned by the validator and are compared by downstream tools and cross-language HED implementations. Renaming them silently breaks any code that matches on the literal string. (See inline comments on both files.)
2	hed/__init__.py	BaseInput removed from the top-level hed namespace without a deprecation shim. Any existing code that does from hed import BaseInput will get ImportError.
3	hed/models/__init__.py	ColumnMapper and DefinitionEntry dropped from hed.models without deprecation. The test files themselves had to be patched to use the private-module paths, confirming real regressions for published users.
4	hed/schema/__init__.py	HedSchemaEntry, UnitClassEntry, UnitEntry, HedTagEntry, and HedSchemaSection removed from hed.schema without deprecation. The new docstrings explicitly advertise these for direct use, making it inconsistent that they are no longer importable from the package.

Important

#	Location	Issue
5	hed/tools/bids/__init__.py	walk_back, get_candidates, and matches_criteria removed from package-level exports without deprecation. The functions still exist in bids_util.py (tests import them directly), but the public from hed.tools.bids import get_candidates path is broken.

---

What to do

The CHANGELOG.md already notes that 1.0.0 is a planned breaking release. These changes belong there, not in an unversioned pre-1.0.0 commit:

1. Either revert the string-value changes and the public-API removals for the current release, adding deprecation re-exports + warnings, then remove them in 1.0.0; or
2. Clearly label this PR as the 1.0.0 breaking release, add a CHANGELOG.md entry that lists every renamed error code and every removed public symbol, confirm alignment with the cross-language HED spec on code-string values, and bump the version accordingly.

The non-breaking parts of this PR (enum, renames without value changes, docstrings, CI fix) can merge as-is once the above is resolved.