-
Notifications
You must be signed in to change notification settings - Fork 7
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!: rename extension_reqs
to runtime_reqs
#1776
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1776 +/- ##
=======================================
Coverage 86.73% 86.73%
=======================================
Files 186 186
Lines 33995 33990 -5
Branches 30870 30865 -5
=======================================
- Hits 29486 29482 -4
+ Misses 2840 2839 -1
Partials 1669 1669
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary
|
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.
Looks good. Can you please explain in the commit message why you have chosen not to rename CustomConst::extension_reqs
?
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.
- Should we rename the
extension_inference
feature? I can't find a good name that's not a mouthful, so we may leave it as is.
We should however update the feature explanation inhugr/README.md
andhugr-core/README.md
@@ -125,7 +125,7 @@ pub(crate) fn collect_signature_exts<RV: MaybeRV>( | |||
used_extensions: &mut ExtensionRegistry, | |||
missing_extensions: &mut ExtensionSet, | |||
) { | |||
// Note that we do not include the signature's `extension_reqs` here, as those refer |
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.
Remove the TODO and issue link below.
@@ -128,7 +128,7 @@ fn resolve_signature_exts( | |||
extensions: &ExtensionRegistry, | |||
used_extensions: &mut ExtensionRegistry, | |||
) -> Result<(), ExtensionResolutionError> { | |||
// Note that we do not include the signature's `extension_reqs` here, as those refer | |||
// Note that we do not include the signature's `runtime_reqs` here, as those refer |
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.
Remove the TODO and issue link below.
I think we should remove this feature, making it always-on. Not here, but I don't think we should spend time looking for a better name |
I don't want to make any decisions about extension_inference now - the spectrum of possibilities has some strong extremes we need to think through slowly and carefully. Have just updated the README. |
Closes #1734
CustomConst::extension_reqs not renamed because they still refer to the extensions where they are defined. this should be updated to references to match ops and types #1775
BREAKING CHANGE:
extension_reqs
field in FunctionType and Extension renamed toruntime_reqs