Open
Conversation
Member
turbomam
commented
Jan 2, 2024
turbomam
commented
- python test for term name length #736
Member
Author
|
This PR errors out as expected ecause there are two terms with names over 20 characters:
|
Member
Author
|
Action item: rename
|
Contributor
LinkML Linting Resultslinkml, version 1.8.4 linting found the following issues. For more information about the linting rule categories, see the LinkML linter documentation. Summary
Problems per SchemaFile: /home/runner/work/mixs/mixs/src/mixs/schema/mixs.yaml Warning
|
Member
Author
2025-04-21Slots whose names are greater than 20 characters: {'nose_mouth_teeth_throat_disord': 30, 'x16s_recover_software': 21}
|
Member
Author
Collaborator
|
@turbomam am I understanding that this PRs checks are failing because the |
turbomam
force-pushed
the
736-python-test-for-term-name-length
branch
from
e07fd81 to
5bcc6d3
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a regression test to enforce a maximum MIxS term (slot) name length of 20 characters (excluding MixsCompliantData domain slots), in response to Issue #736.
Changes:
- Add
tests/test_term_name_length.pyto validate slot-name length constraints viaSchemaViewoversrc/mixs/schema/mixs.yaml. - Update
pyproject.tomlwith a newpoetry = "^1.7.1"entry under[tool.poetry].
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/test_term_name_length.py | Introduces a schema-based test asserting slot names are ≤ 20 chars (with domain-based exclusions). |
| pyproject.toml | Adds a Poetry-related version field to the Poetry project metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+4
to
+5
| import src.mixs.datamodel.mixs as mixs | ||
|
|
There was a problem hiding this comment.
import src.mixs.datamodel.mixs as mixs will raise ModuleNotFoundError in this repo (there is no src/mixs/datamodel/ directory), and it's also unused in the test. Remove this import, or switch to the correct import path used elsewhere (e.g. from mixs.datamodel.mixs import ...) if the intent is to ensure the generated datamodel is importable.
Suggested change
| import src.mixs.datamodel.mixs as mixs |
| #enable = true | ||
| #vcs = "git" | ||
| #style = "pep440" | ||
| poetry = "^1.7.1" |
There was a problem hiding this comment.
poetry = "^1.7.1" is not a valid key under [tool.poetry] and will cause Poetry to treat pyproject.toml as invalid (unknown/extra field). If you need to pin the Poetry tool version, do it in CI/tooling config; otherwise remove this line.
Suggested change
| poetry = "^1.7.1" |
Comment on lines
+21
to
+22
| slot_length_name_limit = 20 | ||
| slot_name_length_ok = [] |
There was a problem hiding this comment.
slot_length_name_limit reads like the slot length's name limit rather than a slot-name length limit. Consider renaming to something like slot_name_length_limit to reduce ambiguity, especially since this value is a policy constant used in the assertion message.
Comment on lines
+22
to
+23
| slot_name_length_ok = [] | ||
| slot_name_length_too_long = {} |
There was a problem hiding this comment.
slot_name_length_ok is populated but never used in the test, which adds noise and makes it harder to see what the test is asserting. Consider removing it (or assert on it) to keep the test focused.
|
|
||
| schema_view = SchemaView(SCHEMA_PATH) | ||
| slots = schema_view.all_slots() | ||
| slot_names = [str(sk) for sk, sv in slots.items()] |
There was a problem hiding this comment.
slot_names = [str(sk) for sk, sv in slots.items()] iterates values but never uses sv. You can simplify this to iterate keys directly (e.g., list(slots) / slots.keys()), which is clearer and avoids an unused loop variable.
Suggested change
| slot_names = [str(sk) for sk, sv in slots.items()] | |
| slot_names = [str(sk) for sk in slots.keys()] |
This was referenced Mar 10, 2026
Member
Author
Rework planThe 20-char limit is confirmed as a hard requirement — INSDC Feature Table Spec Section 3.1, reaffirmed by TWG in #763 (option 3: enforce going forward, leave existing names as-is). So this PR's intent is correct and wanted. What needs to change before merge:
Related
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.