Skip to content
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

feat: if.scikit-build-version #851

Merged
merged 3 commits into from
Aug 6, 2024
Merged

feat: if.scikit-build-version #851

merged 3 commits into from
Aug 6, 2024

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Aug 5, 2024

This adds an if.scikit-build-version, and makes sure it can be used to gate features not yet implemented. This should help with #769 by providing a way to support old scikit-build-core's on Python 3.7 even after we drop support.

Also fixes auto minimum-version to respect markers.

Signed-off-by: Henry Schreiner <[email protected]>
@LecrisUT
Copy link
Collaborator

LecrisUT commented Aug 5, 2024

The plain version naming is so ambiguous though. What about skbuild-version or skbuild-core-version to mirror the CMake variable?

Copy link

codecov bot commented Aug 5, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.87%. Comparing base (0a8a352) to head (54ff64a).
Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
...rc/scikit_build_core/settings/skbuild_overrides.py 91.66% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #851   +/-   ##
=======================================
  Coverage   83.87%   83.87%           
=======================================
  Files          74       74           
  Lines        4347     4361   +14     
=======================================
+ Hits         3646     3658   +12     
- Misses        701      703    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henryiii
Copy link
Collaborator Author

henryiii commented Aug 5, 2024

I was trying to match minimum-version, which also doesn't have any extra qualifier. Other versions, like cmake.version or python-version have qualifiers. That was the thinking, anyway. It's also rather important, so scikit-build-core-version or scikit-build-version seems really long, especially for something this integral. I've avoided "skbuild" in pyproject.toml.

Admittedly, minimum-version is tool.scikit-build.minimum-version, while this is tool.scikit-build.overrides[].if.version, which is a bit more disconnected from the "scikit-build" part.

@henryiii
Copy link
Collaborator Author

henryiii commented Aug 5, 2024

Would if.__version__ be clearer? Or if.core-version? We've mostly used "scikit-build" in the settings, but I could see confusion here if scikit-build-version was used.

@henryiii
Copy link
Collaborator Author

henryiii commented Aug 5, 2024

scikit-build-version isn't terrible either, I suppose.

@LecrisUT
Copy link
Collaborator

LecrisUT commented Aug 5, 2024

while this is tool.scikit-build.overrides[].if.version, which is a bit more disconnected from the "scikit-build" part.

☝️

Length wise I think it's fine as it will always be in its own block with only if. or if.*. prefixed.

scikit-build-version isn't terrible either, I suppose.

👍. Not much longer than python or minimum

@henryiii henryiii changed the title feat: if.version feat: if.scikit-build-version Aug 5, 2024
docs/overrides.md Outdated Show resolved Hide resolved
@henryiii henryiii merged commit 01342be into main Aug 6, 2024
52 checks passed
@henryiii henryiii deleted the henryiii/feat/if.version branch August 6, 2024 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants