Skip to content

feat(sanity): check sample data file only if it is valid #269

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.

Sign up for GitHub

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

Merged
ktro2828 merged 1 commit into from
Apr 9, 2026
Merged

feat(sanity): check sample data file only if it is valid #269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion t4_devkit/sanity/reference/ref201.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,5 @@ def check(self, context: SanityContext) -> list[Reason] | None:
return [
Reason(f"File not found: {record['filename']}")
for record in records
if not data_root.joinpath(record["filename"]).exists()
if record.get("is_valid", True) and not data_root.joinpath(record["filename"]).exists()
Comment on lines 35 to +37
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comprehension can raise a KeyError if a SampleData record is missing the required "filename" key (e.g., malformed JSON): the filter only checks is_valid, but the element expression and exists() path both index record["filename"]. Since checkers are run without exception handling, this can crash the entire sanity run. Consider using record.get("filename") (or similar) and either skip missing filenames or emit a dedicated Reason for missing/empty filename before calling joinpath().

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new is_valid guard changes REF201 behavior (invalid records should no longer trigger missing-file errors), but there’s no test asserting that an is_valid=False record with a missing/invalid filename is ignored. Adding a focused unit test would prevent regressions and also ensure the checker doesn’t error on intentionally-invalid records.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability/consistency with REF202, consider formatting this condition across multiple lines (or extracting is_valid/filename to locals). The current single-line comprehension is harder to scan and maintain as more guards get added.

Copilot uses AI. Check for mistakes.
] or None
3 changes: 2 additions & 1 deletion t4_devkit/sanity/reference/ref202.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ def check(self, context: SanityContext) -> list[Reason] | None:
return [
Reason(f"File not found: {record['info_filename']}")
for record in records
if record.get("info_filename") is not None
if record.get("is_valid", True)
and record.get("info_filename") is not None
and not data_root.joinpath(record["info_filename"]).exists()
Comment on lines +37 to 39
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new is_valid guard changes REF202 behavior (invalid records should be ignored), but there’s no test asserting that an is_valid=False record with a missing/nonexistent info_filename does not produce a failure. Adding a unit test for this case would prevent future regressions.

Copilot uses AI. Check for mistakes.
] or None
Loading