-
Notifications
You must be signed in to change notification settings - Fork 76
feat: Add a setting to disallow hard-coding some setting keys in the pyproject.toml
#1078
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
a2504cc to
9922ae6
Compare
9a77549 to
417a6e7
Compare
53aed69 to
f1a073a
Compare
f1a073a to
4c114df
Compare
4c114df to
404adc4
Compare
|
Following discussion with @henryiii during Note from community meeting. Ideally, the schema should be updated before integrating. |
Used for further validations Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Add `test_disallow_hardcoded` to cover these type of settings Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
982f3fc to
d699403
Compare
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 adds a mechanism to disallow hard-coding certain settings keys (like fail) in the top-level pyproject.toml, while still allowing them in override contexts. The fail field is the first to use this new override_only metadata flag.
Key Changes
- Introduced
override_onlymetadata field for settings that should only appear in overrides - Added validation logic to check for hard-coded
override_onlyfields and warn/error accordingly - Updated
failfield to useOptional[bool]with defaultNoneand marked asoverride_only
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/scikit_build_core/settings/skbuild_model.py |
Changed fail field to Optional[bool] with override_only=True metadata |
src/scikit_build_core/settings/skbuild_overrides.py |
Added OverrideRecord dataclass and record_override function to track override history |
src/scikit_build_core/settings/skbuild_read_settings.py |
Added _validate_overrides function to validate override_only fields |
src/scikit_build_core/settings/skbuild_schema.py |
Updated schema generation to exclude override_only properties from top-level schema |
src/scikit_build_core/settings/json_schema.py |
Added scikit-build:override-only marker to JSON schema properties |
src/scikit_build_core/settings/documentation.py |
Added override_only field to DCDoc dataclass |
src/scikit_build_core/settings/skbuild_docs_readme.py |
Filtered out override_only items from README documentation |
src/scikit_build_core/resources/scikit-build.schema.json |
Removed default: false from fail property and updated schema structure |
tests/test_settings_overrides.py |
Added test for disallow hardcoded functionality |
tests/test_settings_docs.py |
Updated assertion to verify fail is not in documentation |
docs/reference/configs.md |
Removed default value from fail configuration documentation |
README.md |
Removed fail = false from example configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Henry Schreiner <[email protected]>
d699403 to
e946c4e
Compare
|
Note this is a very slight breaking change; if someone put |
This complements the discussion in #1050 that we should not allow some fields to be hard-coded. We should allow them in the context of overrides, so here is a rather convoluted way of adding that check.
This will also make it possible to extend it further if we want more specific checks for specific overrides matchers as well. E.g. we can add a restriction that
build.requirescannot have@in ansdist