Refactored actions to use uvx, eliminated black, removed various shims#1251
Refactored actions to use uvx, eliminated black, removed various shims#1251VisLab merged 5 commits intohed-standard:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates CI workflows to run Python tooling via uvx and significantly expands schema comparison test coverage, including new assertions around schema “extras” DataFrame diffing.
Changes:
- Switch GitHub Actions steps for Ruff/Black/Codespell/Mdformat to use
uvxinstead of creating a venv and installing tools manually. - Add extensive unit tests for
SchemaComparer(matching tags, schema diffs, extras DataFrame comparisons, pretty-print formatting). - Adjust
SchemaComparerextras comparison to ignore the internalin_librarycolumn during key/column selection.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/schema/test_schema_compare.py |
Large expansion/refactor of tests for SchemaComparer, including extras DataFrame diffing and output formatting checks. |
hed/scripts/schema_script_util.py |
Docstring updates to reflect filtering out warnings (typo introduced). |
hed/schema/schema_compliance.py |
Removes backward-compatibility shim module. |
hed/schema/schema_attribute_validators.py |
Removes backward-compatibility shim module. |
hed/schema/schema_comparer.py |
Adds section-name constants for extras and updates extras diffing to exclude the in_library column. |
.github/workflows/ruff.yaml |
Runs Ruff linter + formatter check via uvx. |
.github/workflows/mdformat.yaml |
Migrates mdformat to uvx (currently broken YAML/script). |
.github/workflows/codespell.yaml |
Runs codespell via uvx. |
.github/workflows/black.yaml |
Runs Black via uvx. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR Review: Used uvx in some actionsSummaryThe CI workflow simplification (replacing venv+pip with uvx) is a good idea. The schema comparer cleanups and expanded test suite are solid. However there are two bugs in the workflow files that will break CI. Criticalmdformat.yaml — shell step is broken The black.yaml — missing newline at end of file The file ends without a trailing newline. POSIX requires text files to end with a newline; some tools will flag this. Importanthed/scripts/schema_script_util.py line 38 — docstring typo "non-warningcompliance issues" is missing a space; should be "non-warning compliance issues". Suggestions
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
VisLab
changed the title
PR ReviewThis PR refactors CI workflows to use Critical
The - name: Run mdformat (check only)
run: uvx mdformat --check --wrap no --number docs/ *.md(or keep ImportantDocstring typo in Line in the new docstring reads Deletion of backward-compat shims
Suggestions
|
PR Review: Refactored actions to use uvx, eliminated black, removed various shimsOverall, this is a clean CI modernization — switching to Important1. The previous workflow explicitly installed Fix: 2. Shim removal ( Both files were explicitly marked as backward-compatible shims with docstrings pointing users to the canonical import paths. Their removal is a breaking change for any downstream code importing from Suggestion
Running No issues found in
|
PR Review: Refactored actions to use uvx, eliminated black, removed various shimsSummaryThe CI/workflow changes are clean and well-structured. The source code cleanups (f-string merges, blank-line removal) are all correct and no-op. The new test suite for Important1. Duplicate string constants (
from hed.schema.schema_io.df_constants import SOURCES_KEY, PREFIXES_KEY, EXTERNAL_ANNOTATION_KEY
class SchemaComparer:
SOURCES = SOURCES_KEY
PREFIXES = PREFIXES_KEY
ANNOTATION_PROPERTY_EXTERNAL = EXTERNAL_ANNOTATION_KEY2. Removal of
Suggestion3. Extras-section deletions classified as In 4.
|
| f"Invalid tag {tag} found in definition for {def_name}. " | ||
| f"Def, Def-expand, and Definition tags cannot be in definitions." | ||
| ) | ||
| return f"Invalid tag {tag} found in definition for {def_name}. Def, Def-expand, and Definition tags cannot be in definitions." |
There was a problem hiding this comment.
Important: This line is ~131 characters, exceeding both the old 127-char limit and the new 120-char ruff limit set in this PR. The previous multi-line format was correct:
| return f"Invalid tag {tag} found in definition for {def_name}. Def, Def-expand, and Definition tags cannot be in definitions." | |
| return ( | |
| f"Invalid tag {tag} found in definition for {def_name}. " | |
| f"Def, Def-expand, and Definition tags cannot be in definitions." | |
| ) |
| f"Tag '{tag}' has annotation '{annotation_value}' with prefix '{prefix}' " | ||
| f"that is not defined in the Prefixes section." | ||
| ) | ||
| return f"Tag '{tag}' has annotation '{annotation_value}' with prefix '{prefix}' that is not defined in the Prefixes section." |
There was a problem hiding this comment.
Important: This line is ~130 characters, exceeding both the old 127-char limit and the new 120-char ruff line-length set in this PR. The previous multi-line format was correct:
| return f"Tag '{tag}' has annotation '{annotation_value}' with prefix '{prefix}' that is not defined in the Prefixes section." | |
| return ( | |
| f"Tag '{tag}' has annotation '{annotation_value}' with prefix '{prefix}' " | |
| f"that is not defined in the Prefixes section." | |
| ) |
| self.extras = ( | ||
| {} | ||
| ) # Used to store any additional data that might be needed for serialization (like OWL or other formats) | ||
| self.extras = {} # Used to store any additional data that might be needed for serialization (like OWL or other formats) |
There was a problem hiding this comment.
Important: This line is 128 characters (exceeds both the old 127-char and new 120-char limits). The original three-line form kept the comment readable:
| self.extras = {} # Used to store any additional data that might be needed for serialization (like OWL or other formats) | |
| self.extras = ( | |
| {} | |
| ) # Used to store any additional data that might be needed for serialization (like OWL or other formats) |
Alternatively, shorten or move the comment to a preceding line.
| line-ending = "auto" | ||
|
|
||
| [tool.black] | ||
| line-length = 127 |
There was a problem hiding this comment.
Important: [tool.ruff] line-length is now 120 (line 122) but [tool.black] still says 127. Since black is removed from dev dependencies and CI in this PR, the entire [tool.black] section should be removed to avoid confusion about the enforced line length.
| _, not_in_1, not_in_2, unequal = comp.compare_schemas(attribute_filter=HedKey.InLibrary) | ||
| self.assertEqual(len(not_in_1[HedSectionKey.Tags]), 21) | ||
| self.assertEqual(len(not_in_2[HedSectionKey.Tags]), 10) | ||
| self.assertEqual(len(unequal[HedSectionKey.Tags]), 80) |
There was a problem hiding this comment.
Suggestion: The expected counts (21, 10, 80) are tightly coupled to the internal content of specific released schema versions. If those schemas are ever regenerated or the comparison logic changes, these assertions will fail with no obvious reason. At minimum, add a comment documenting where these numbers come from and that they are intentionally pinned.
| @@ -24,13 +24,5 @@ jobs: | |||
| enable-cache: true | |||
| cache-dependency-glob: "**/pyproject.toml" | |||
|
|
|||
There was a problem hiding this comment.
Suggestion: Two separate uvx ruff invocations fetch and install ruff twice. Prefer a single step that calls both sub-commands:
| run: uvx ruff check . && uvx ruff format --check . |
(No change needed if that is acceptable to you — it works correctly, just slightly redundant.)
PR Review: Refactored actions to use uvx, eliminated black, removed various shimsSummaryThe CI infrastructure changes (switching to Two issues need fixing before merge. Important1. Line-length regressions in error message files Three error message return statements that were previously correctly split across multiple lines have been collapsed into single lines of 128–131 characters, exceeding both the old 127-char limit and the new 120-char ruff
Inline suggestions posted on each line. 2.
Suggestions
|
No description provided.