gh67 - xts validate#71
Open
zghp wants to merge 3 commits into
Open
Conversation
|
I have read the CLA Document and I hereby sign the CLA zghp seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
There was a problem hiding this comment.
Pull request overview
Adds a built-in xts validate subcommand to validate .xts files (YAML + basic structural checks) without running commands, along with documentation and tests.
Changes:
- Add CLI handling for
xts validate <file.xts>and implement.xtsfile validation logic. - Add pytest coverage for validate success and common failure modes.
- Document the new validate command in the README.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/test_xts_all_cases.py | Adds tests covering xts validate success and error cases (missing file, YAML error, invalid structure). |
| src/xts_core/xts.py | Implements validate command routing and validation helpers for .xts syntax/structure. |
| README.md | Documents xts validate usage and expected exit codes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zghp
force-pushed
the
feature/gh67-xts-validate
branch
from
July 2, 2026 11:38
2278d83 to
3ca1bb2
Compare
Comment on lines
+393
to
+407
| args, remaining_args = parser.parse_known_args() | ||
| args = vars(args) | ||
| alias_name = args.get('alias_name') | ||
| match alias_name: | ||
| case 'alias': | ||
| alias_name_subparser = list(filter(lambda x: x.dest == 'alias_name',parser._actions))[0] | ||
| alias_subparser = alias_name_subparser.choices.get('alias') | ||
| xts_alias.run_alias_builtin(alias_subparser) | ||
| case 'validate': | ||
| self._run_validate_command(remaining_args if remaining_args else [args.get('path', '')]) | ||
| case None|'alias_name': | ||
| parser.print_help() | ||
| raise SystemExit(0) | ||
| case _: | ||
| self._run_yaml_runner(alias_name, remaining_args) |
Comment on lines
+228
to
+237
| args, remaining_args = parser.parse_known_args() | ||
| args = vars(args) | ||
| alias_name = args.get('alias_name') | ||
| if alias_name == 'alias': | ||
| alias_name_subparser = list(filter(lambda x: x.dest == 'alias_name',parser._actions))[0] | ||
| alias_subparser = alias_name_subparser.choices.get('alias') | ||
| xts_alias.run_alias_builtin(alias_subparser) | ||
| else: | ||
| # Attempt to run the yaml alias; if resolution fails, error will be raised | ||
| self._run_yaml_runner(alias_name, remaining_args) |
Comment on lines
+365
to
+368
| def _run_completion(self, arg_parser:XTSArgumentParser): | ||
| os.environ['_ARGPARSE_COMPLETE'] = os.getenv('_XTS_COMPLETE') | ||
| completion = argparse_completion.get_completion(arg_parser) | ||
| if 'alias_name' in completion: |
Comment on lines
+441
to
+445
| if len(sys.argv) < 3: | ||
| alias_parser.print_help() | ||
| print("\nCurrent aliases:") | ||
| list_aliases() | ||
| return 0 | ||
|
|
||
| args = alias_parser.parse_args(argv) | ||
|
|
||
| if args.list: | ||
| list_aliases() | ||
| return 0 | ||
|
|
||
| if args.remove is not None: | ||
| if remove_alias(args.remove): | ||
| print(f"Removed alias: {args.remove}") | ||
| return 0 | ||
| print(f"Alias not found: {args.remove}") | ||
| return 2 | ||
|
|
||
| if args.refresh is not None: | ||
| try: | ||
| name, cached_path = refresh_alias(args.refresh) | ||
| print(f"Refreshed alias: {name} -> {cached_path}") | ||
| raise SystemExit(0) | ||
| args_dict = vars(alias_parser.parse_args(sys.argv[2:])) | ||
| action = args_dict.get('alias_action') |
Comment on lines
+31
to
+35
| # This ensures data/ is included in sdist/wheel | ||
| [tool.setuptools.data-files] | ||
| # Installs data into a shared location in site-packages | ||
| "xts_core_data" = ["data/**/*"] | ||
|
|
Comment on lines
+329
to
+332
| self._xts_config = data | ||
| self._command_sections = self._get_command_sections() | ||
| self._validate_xts_structure(data) | ||
| if self._ignored_sections: |
Comment on lines
165
to
+169
| for key, value in self._xts_config.items(): | ||
| if isinstance(value, dict) and _is_command_section(value): | ||
| command_sections[key] = value | ||
| if isinstance(value, dict): | ||
| if _is_command_section(value): | ||
| command_sections[key] = value | ||
| else: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
src/xts_core/xts.py:392
argparsesubparser aliases causeargs.alias_nameto be the canonical subparser name ("alias_name") rather than the actual alias token (e.g.myalias). As written,xts myalias ...will hit thecase None|'alias_name'branch (printing help) and never run the YAML runner. Also, thealiassubcommand ignores the return code fromrun_alias_builtin, soxts alias remove missingwill still exit 0.
args, remaining_args = parser.parse_known_args()
args = vars(args)
alias_name = args.get('alias_name')
match alias_name:
case 'alias':
src/xts_core/xts.py:365
_run_completion()assumesargparse_completionimported successfully and thatCOMP_WORDSis set and has at least two tokens. In environments where the dependency isn't installed (despite the guarded import) or where bash doesn't provideCOMP_WORDS, this will raiseAttributeError/IndexErrorduring completion.
os.environ['_ARGPARSE_COMPLETE'] = os.getenv('_XTS_COMPLETE')
completion = argparse_completion.get_completion(arg_parser)
if 'alias_name' in completion:
completion.remove('alias_name')
if len(completion) == 0 and (comp_words:=(os.getenv('COMP_WORDS').split()))[1] in xts_alias.load_aliases().keys():
src/xts_core/xts.py:267
_validate_xts_structure()currently rejects any list outside of acommandkey. This makesxts validatereject real-world configs that intentionally include ignored list sections (e.g.examples/hello_world.xtshasunrecognised:as a list). Validation should only reject lists when they are being used to define commands/command trees, while allowing lists in sections that don't containcommandkeys.
if key == 'command':
self._validate_command_value(value, node_path)
elif isinstance(value, list):
raise ValueError(
f'Invalid .xts structure at "{node_path}": lists are not supported '
src/xts_core/xts.py:169
_get_command_sections()only records ignored sections when the top-level value is adict. Non-dict top-level sections (including list sections that will also be ignored at runtime) won't be included in_ignored_sections, soxts validatewon't warn about them even though they are ignored.
for key, value in self._xts_config.items():
if isinstance(value, dict):
if _is_command_section(value):
command_sections[key] = value
else:
pyproject.toml:34
[tool.setuptools.data-files]referencesdata/**/*, but there is no top-leveldata/directory in the repo (the script is undersrc/xts_core/data/). This can break sdist/wheel builds when setuptools tries to include non-existent files. Since you already include package data viaxts_core = ["data/**/*"], removing thedata-filesstanza avoids the build-time failure.
# This ensures data/ is included in sdist/wheel
[tool.setuptools.data-files]
# Installs data into a shared location in site-packages
"xts_core_data" = ["data/**/*"]
src/xts_core/xts.py:227
- The legacy
_parse_first_arg()wrapper has the same alias-resolution issue asrun(): when an alias is provided via thealias_namesubparser,args['alias_name']becomes the literal string"alias_name", so_run_yaml_runner()will try to resolve an alias namedalias_name. Also, thealiaspath should propagate the return code (currently it just callsrun_alias_builtin()and keeps going).
args, remaining_args = parser.parse_known_args()
args = vars(args)
alias_name = args.get('alias_name')
if alias_name == 'alias':
alias_name_subparser = list(filter(lambda x: x.dest == 'alias_name',parser._actions))[0]
Comment on lines
+441
to
+445
| if len(sys.argv) < 3: | ||
| alias_parser.print_help() | ||
| print("\nCurrent aliases:") | ||
| list_aliases() | ||
| return 0 | ||
|
|
||
| args = alias_parser.parse_args(argv) | ||
|
|
||
| if args.list: | ||
| list_aliases() | ||
| return 0 | ||
|
|
||
| if args.remove is not None: | ||
| if remove_alias(args.remove): | ||
| print(f"Removed alias: {args.remove}") | ||
| return 0 | ||
| print(f"Alias not found: {args.remove}") | ||
| return 2 | ||
|
|
||
| if args.refresh is not None: | ||
| try: | ||
| name, cached_path = refresh_alias(args.refresh) | ||
| print(f"Refreshed alias: {name} -> {cached_path}") | ||
| raise SystemExit(0) | ||
| args_dict = vars(alias_parser.parse_args(sys.argv[2:])) | ||
| action = args_dict.get('alias_action') |
zghp
force-pushed
the
feature/gh67-xts-validate
branch
from
July 20, 2026 10:39
057a20d to
c1379a6
Compare
TB-1993
requested changes
Jul 20, 2026
Comment on lines
+50
to
+53
| try: | ||
| import argparse_completion | ||
| except Exception: | ||
| argparse_completion = None |
Contributor
There was a problem hiding this comment.
This should just stay as import argpare_completion. The import error you get if it's not installed should be fatal.
| return first_arg_parser | ||
|
|
||
| # Backwards-compatible wrapper expected by older tests | ||
| def _parse_first_arg(self): |
Contributor
There was a problem hiding this comment.
This method was deliberately removed. The test needs upgrading, it shouldn't be put back.
Comment on lines
+283
to
+297
| if not argv or len(argv) == 0 or not argv[0]: | ||
| validate_help_parser = XTSArgumentParser( | ||
| prog='xts validate', | ||
| description='Validate an .xts file and report syntax issues', | ||
| ) | ||
| validate_help_parser.add_argument( | ||
| 'path', | ||
| nargs='?', | ||
| help='Path to the .xts file to validate', | ||
| ) | ||
| validate_help_parser.print_help() | ||
| print('Example: xts validate examples/example.xts') | ||
| raise SystemExit(1) | ||
|
|
||
| if argv[0] in {'-h', '--help'}: |
Contributor
There was a problem hiding this comment.
What are these if statements for? Why aren't we just letting argparse handle the sys.argv?
Comment on lines
+245
to
+256
| if isinstance(value, str): | ||
| _validate_shell_command(value, path) | ||
| return | ||
|
|
||
| if isinstance(value, list): | ||
| if not all(isinstance(item, str) for item in value): | ||
| raise ValueError(f'Invalid command list at "{path}": all entries must be strings') | ||
| for item in value: | ||
| _validate_shell_command(item, path) | ||
| return | ||
|
|
||
| raise ValueError(f'Invalid command definition at "{path}": expected a string or list of strings') |
Contributor
There was a problem hiding this comment.
This would be better as an if/elif/else so we have a clear path to the return, rather than multipl returns.
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.
No description provided.