Analyze errors, tools, and validator modules for structure and efficiency

[VisLab]

We are getting ready to make the v1.0.0 release which will be a breaking change. @claude, look over at the submodules and evaluate the structure and efficiency and check if there are things to improve. Focus on the errors, tools, and validator modules.

[github-actions]

Claude finished @VisLab's task in 4m 11s —— View job

---

Code Review: errors, tools, and validator modules

Tasks:

• Gather context and understand the request
• Analyze hed/errors/ module structure and efficiency
• Analyze hed/tools/ module structure and efficiency
• Analyze hed/validator/ module structure and efficiency
• Compile findings and recommendations

---

Summary

The three modules are generally well-structured with clear separation of concerns. The architecture is sound and the overall design is appropriate for the domain. What follows are the confirmed issues worth addressing before v1.0.0, organized by severity.

---

Confirmed Bugs

1. hed/tools/analysis/hed_tag_counts.py:146 — TypeError bug

The update loop for value_dict applies the merge incorrectly — it adds val_count to the whole dict instead of to the specific value:

# Bug (line 146): adds dict + val_count (TypeError at runtime)
self.tag_dict[tag].value_dict[value] = self.tag_dict[tag].value_dict + val_count

# Should be:
self.tag_dict[tag].value_dict[value] = self.tag_dict[tag].value_dict[value] + val_count

This will raise a TypeError whenever merge_tag_dicts() is called on tags with value dicts.

2. hed/tools/util/io_util.py:282 — make_path() missing return statement

The function creates the directory correctly but never returns anything. The filename parameter is also never used, making the function's return value always None:

def make_path(root_path, sub_path, filename):
    dir_path = os.path.realpath(os.path.join(root_path, sub_path))
    os.makedirs(dir_path, exist_ok=True)
    # Missing: return os.path.join(dir_path, filename)

3. hed/errors/error_messages.py:271,286 — Missing closing quotes in f-strings

Two error message functions have unclosed string quotes, meaning the closing ' is missing from the message:

# Line 271 — closing quote missing after {tag}
return f"A def tag does not take a placeholder value, but was given one.  Definition: '{tag}"
# Line 286
return f"A Def-expand tag does not take a placeholder value, but was given one.  Definition: '{tag}"
# Should be: ...Definition: '{tag}'"

---

hed/errors/ Module

Overall quality: Good. The decorator-based error registration pattern is elegant and the separation of error types, messages, and the reporter is clean.

Issues

exceptions.py — Multiple constants with identical string values
Several distinct constants all map to the same error code string, making it hard to distinguish why something failed:

SCHEMA_HEADER_MISSING = "SCHEMA_HEADER_INVALID"
SCHEMA_HEADER_INVALID = "SCHEMA_HEADER_INVALID"
SCHEMA_UNKNOWN_HEADER_ATTRIBUTE = "SCHEMA_HEADER_INVALID"

Same pattern for SCHEMA_LIBRARY_INVALID (7 constants → same string) and SCHEMA_LOAD_FAILED (2 constants → same string). The HedExceptions class can distinguish at the Python level, but downstream consumers of the error code string lose that distinction. Consider whether the individual codes need distinct strings for any tooling.

exceptions.py — Inconsistent error code naming conventions
The string values mix camelCase with UPPER_SNAKE_CASE:

FILE_NOT_FOUND = "fileNotFound"       # camelCase value
BAD_PARAMETERS = "badParameters"      # camelCase value
GENERIC_ERROR = "GENERIC_ERROR"       # UPPER_SNAKE_CASE value
INVALID_HED_FORMAT = "INVALID_HED_FORMAT"  # UPPER_SNAKE_CASE value

The older constants use camelCase values; newer ones use UPPER_SNAKE_CASE. This is likely backward-compatibility debt, but worth documenting explicitly.

error_reporter.py:398 — TODO comment indicating unresolved technical debt

# Todo: Move this functionality somewhere more centralized.

Should be tracked as a follow-up issue or addressed before 1.0.

error_reporter.py — Late imports at module level
error_messages and schema_error_messages are imported after class definitions (necessary to avoid circular imports). The comment exists but the pattern is non-obvious — a module-level note explaining why would help maintainers.

---

hed/tools/ Module

Overall quality: Good. The analysis/bids/util separation is logical, and the class hierarchy for tag counts and type factors is well-conceived.

Issues

hed_type_factors.py:66-68 — ValueError raised with wrong signature

raise ValueError(
    "BadFactorEncoding", f"{factor_encoding} is not ..."
)

ValueError with two positional args stores them as a tuple in args — the message is not accessible as str(e). This is also inconsistent with the rest of the codebase, which raises HedFileError for domain-level errors.

hed_type_factors.py:78 — Docstring typo

Return:
    pd.ataFrame:  Contains one-hot representation of requested levels.

Should be pd.DataFrame.

spreadsheet_validator.py:23 — Confusing constant expression

ONSET_TOLERANCE = 10 - 7  # evaluates to 3

This should be written as ONSET_TOLERANCE = 3 (or 1e-7 if a small float was intended rather than 10 - 7).

char_util.py:10 — Fragile relative path

CLASS_REX_FILENAME = "../data/class_regex.json"

This path is relative to the calling location, not the module file. Should use os.path.join(os.path.dirname(__file__), "../data/class_regex.json") or importlib.resources.

annotation_util.py — Two blocks of commented-out dead code
Lines 363–391 and 471–489 contain entire commented-out functions (_get_row_tags, _update_remainder). These should be removed; git history preserves them if needed.

annotation_util.py — Near-duplicate functions to_factor() / series_to_factor()
Both functions perform factor encoding on HED data — one takes a list, the other a Series — but the logic is essentially the same. Worth consolidating or clearly documenting the distinction.

hed_type_manager.py — Encapsulation violation
_type_map (a private dict) is accessed directly in at least two external call sites. A getter would be safer and more maintainable.

---

hed/validator/ Module

Overall quality: Good. The delegation pattern (HedValidator → CharValidator, TagValidator, GroupValidator, etc.) is well thought out and the error context push/pop pattern in ErrorHandler integrates cleanly throughout.

Issues

spreadsheet_validator.py:23 — ONSET_TOLERANCE (same as above)

sidecar_validator.py:96,169 — Duplicate regex pattern
The pattern r"\{([a-z_\-0-9]+)\}" appears twice in the same file. It should be a module-level constant:

COLUMN_REF_PATTERN = re.compile(r"\{([a-z_\-0-9]+)\}", re.IGNORECASE)

sidecar_validator.py:102 — Silent truncation with strict=False

ref_dict = dict(zip(refs, combination, strict=False))

If refs and combination are different lengths, this silently drops items. The comment at line 95 says # TODO: Figure out why this pattern is giving lint errors. — if strict=True triggers a bug, it warrants investigation rather than suppression.

Dead code — commented-out blocks in multiple validator files

• def_validator.py:119-121: Commented-out unit validation call
• group_util.py:45,128: Commented-out validator calls
  These should be removed or converted to tracked issues.

def_validator.py:142-185 — validate_onset_offset() is doing too much
The method handles onset, offset, and inset all in one long block of nested conditions. Extracting one method per temporal tag type would significantly improve readability.

reserved_checker.py:134 — float("-inf") as initial value

min_allowed = float("-inf")

The intent is "unbounded minimum." Using float("-inf") is correct but surprising in this context. A comment explaining this sentinel value would help.

onset_validator.py — Stateful validator not reset between calls
_onsets is instance state that accumulates across calls. If the same OnsetValidator instance is reused (e.g., across multiple files), the state from the previous call persists. The class should either reset in validate_temporal_relations() or document that each instance is single-use.

---

Cross-Cutting Concerns

Inconsistent error type usage in tools/
temporal_event.py raises a plain ValueError rather than HedFileError, while the rest of the tools module consistently uses HedFileError. A global HedFileError policy for domain errors would improve consistency.

TODO comments as technical debt
There are at least 3 unresolved TODO comments across the modules (error_reporter.py:398, sidecar_validator.py:95, def_validator.py region). These should either be addressed or converted to tracked GitHub issues before 1.0.

Type hints are inconsistently applied
Many public methods across all three modules have type hints; many do not. For a 1.0 release, consistency in the public API surface (at least) would be valuable.

---

Priority Summary

Priority	File	Issue
Bug	hed_tag_counts.py:146	value_dict + val_count TypeError
Bug	io_util.py:282	make_path() missing return + unused filename param
Bug	error_messages.py:271,286	Unclosed f-string quotes in error messages
Medium	exceptions.py	Multiple constants with same string value; mixed naming convention
Medium	hed_type_factors.py:66	ValueError raised with wrong signature
Medium	char_util.py:10	Fragile relative path for class_regex.json
Minor	spreadsheet_validator.py:23	ONSET_TOLERANCE = 10 - 7 should be = 3 (or = 1e-7)
Minor	sidecar_validator.py:96,169	Duplicate regex pattern
Minor	onset_validator.py	Stateful validator not reset between calls
Minor	Multiple files	Commented-out dead code blocks
Minor	Multiple files	Unresolved TODO comments
Minor	hed_type_factors.py:78	Docstring typo (pd.ataFrame)

[VisLab]

PR #1247 has addressed this review.