-
Notifications
You must be signed in to change notification settings - Fork 82
♻️ Move needs_statuses and need_tags checking to schema validation
#1605
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
Change `extra_option` schema validation to just `option` validation, which included both core and extra options,
and convert e.g. `needs_statuses = ["a", "b"]` to `schema = {"type": "string", "enum": ["a", "b"]}` and `needs_tags = ["a", "b"]` to `schema = {"type": array", "items": {"type": "string", "enum": ["a", "b"]}}`
This will facilitate #1547
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 refactors schema validation by moving needs_statuses and needs_tags checking from explicit validation logic to JSON schema-based validation. The changes rename extra_option schema validation to option validation (covering both core and extra options) and automatically generate schema constraints from the needs_statuses and needs_tags configuration.
Key changes:
- Removed manual validation checks for
needs_statusesandneeds_tagsin favor of schema-based validation - Renamed
validate_extra_optionstovalidate_option_fieldsand expanded scope to include core fields - Updated schema generation to automatically inject enum constraints for status and tags fields
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_broken_docs.py | Updated test assertions to reflect new schema validation error format for status and tags violations |
| tests/schema/snapshots/test_schema.ambr | Updated snapshot test expectations to reflect renamed validation paths and subtypes |
| sphinx_needs/schema/process.py | Refactored to use validate_option_fields instead of validate_extra_options and include core fields in validation |
| sphinx_needs/schema/core.py | Renamed function and updated schema path/rule names from extra_option to option |
| sphinx_needs/schema/config.py | Renamed enum values from extra_option_* to option_* for consistency |
| sphinx_needs/needs.py | Added logic to merge needs_statuses and needs_tags config into field schemas as enum constraints |
| sphinx_needs/api/need.py | Removed explicit validation logic for status and tags that is now handled by schema validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ubmarco
left a comment
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.
LGTM. One comment, and I also see a Copilot message.
| test_app.build() | ||
| assert get_warnings(test_app) == [ | ||
| "<srcdir>/index.rst:11: WARNING: Need could not be created: Status 'NOT_ALLOWED' not in 'needs_statuses'. [needs.create_need]" | ||
| "ERROR: Need 'SP_TOO_002' has schema violations:", |
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.
This makes SN more resilient, but the error also appears later in the build process, right?
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.
It does appear later yes, but that it actually more "correct", since otherwise the validation is wrong, if e.g. the status is a dynamic function, etc.
It is of note that these warnings no longer include "source mapping" to the original need, this may be a consideration for all of these schema warnings?
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.
Yes, that should totally be there, I thought about this before.
We know the affected need ID, so should not be too hard.
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.
okie, well if you want to maybe open an issue for that, or do the PR yourself, but otherwise I just keep it in my internal todo list lol
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.
Here we go #1606
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1605 +/- ##
==========================================
+ Coverage 86.87% 88.15% +1.27%
==========================================
Files 56 70 +14
Lines 6532 9685 +3153
==========================================
+ Hits 5675 8538 +2863
- Misses 857 1147 +290
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR refactors
needs_statusesandneeds_tagschecking, from explicit validation logic (at need creation time) to the new generic schema-based validation system.The changes rename
extra_optionschema validation tooptionvalidation (covering both core and extra options) and automatically generate schema constraints from theneeds_statusesandneeds_tagsconfiguration, for example:needs_statuses = ["a", "b"]to{"status": {"type": "string", "enum": ["a", "b"]}}andneeds_tags = ["a", "b"]to{"tags": {"type": array", "items": {"type": "string", "enum": ["a", "b"]}}}This fixes issues with these being evaluated too early and not accounting for dynamic functions, needextend, etc ...
and will also facilitate #1547