-
Notifications
You must be signed in to change notification settings - Fork 1.1k
improve XLS validator #457
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR tightens XLS validation to catch more non-compliant specs (notably cases like #451), by centralizing and expanding preamble metadata validation and strengthening Amendment template enforcement.
Changes:
- Added shared preamble validation (
validate_xls_preamble) with stricter required fields, enums, and date-format checks. - Updated the template validator to reuse shared preamble validation and added heuristics to detect “existing XRPL primitive” sections that should use Amendment template headings.
- Updated several existing XLS READMEs to comply with the stricter metadata requirements (status, description, withdrawal reason, etc.).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/xls_parser.py | Introduces shared preamble validation and uses it during full-repo XLS parsing validation. |
| scripts/validate_xls_template.py | Reuses shared preamble validation; adds stricter structure checks and heuristic enforcement for legacy-style Amendment specs. |
| XLS-0101-smart-contracts/README.md | Adjusts status to comply with allowed status values. |
| XLS-0051-nftoken-escrow/README.md | Adds required description metadata and reorganizes preamble fields. |
| XLS-0017-xfl/README.md | Fixes misspelled description metadata key. |
| XLS-0016-nft-metadata/README.md | Normalizes metadata formatting and adds required description. |
| XLS-0015-concise-tx-id/README.md | Adds required withdrawal-reason for Withdrawn status. |
| XLS-0010-non-transferable-tokens/README.md | Adds required description metadata. |
| XLS-0008-tickets/README.md | Adds required withdrawal-reason for Withdrawn status. |
Comments suppressed due to low confidence (2)
scripts/validate_xls_template.py:193
- The section-number stripping regex only matches single-level numbering like
1. Title. Many specs use nested numbering (e.g.1.1. Abstract), which will prevent required-section checks like"Abstract" not in section_titlesfrom working and can yield false validation errors. Consider updating the regex to strip\d+(\.\d+)*.prefixes (or relaxing required-section matching to use substring checks).
def _parse_sections(self):
"""Parse all sections from the markdown content using markdown-it-py."""
scripts/validate_xls_template.py:658
- 'except' clause does nothing but pass and there is no explanatory comment.
print("Validation complete:")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
The template checker is expected to fail here |
Co-authored-by: Vito Tumas <[email protected]>
High Level Overview of Change
This PR improves the XLS validator to better catch non-compliant specs.
Checking XLS headers is now cleaner and DRY-er, lots of code cleanup (removing unused variables etc) as well.
Context of Change
It didn't flag on #451 when it should have
Type of Change