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

Add explicit scenarios for feature gate usage #387

Merged

Conversation

vados-cosmonic
Copy link
Contributor

This commit addds some explanation of the intended usage pattern of feature gates in order to make the transition points and functionality easier to identify/reason about.

Resolves #382

@vados-cosmonic
Copy link
Contributor Author

@alexcrichton / @lukewagner / @yoshuawuyts would anyone mind taking a look at this?

design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for doing this! Just one question below which, depending on what folks here think, suggests a few more changes in the PR.

design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Show resolved Hide resolved
@vados-cosmonic vados-cosmonic force-pushed the explicit-feature-gate-scenarios branch 3 times, most recently from 4c288a4 to 29682a4 Compare August 19, 2024 15:42
design/mvp/WIT.md Outdated Show resolved Hide resolved
Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

design/mvp/WIT.md Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
@lukewagner
Copy link
Member

I just realized that one other change we discussed in #382 that maybe we want to make in this PR (if we want it to fully resolve #382) is the validation requirements (attached to the various gates) of: @since xor @unstable and gates-imply-top-level-package (just as an extra sentence or two in the section that introduces the gates).

@vados-cosmonic
Copy link
Contributor Author

@lukewagner thanks that's a great catch -- that little requirement was definitely an eye opener on the code side -- will add!

@vados-cosmonic
Copy link
Contributor Author

Kept this particular change separate so it might be easy to review (I added some headings):

af0c60e

Would appreciate just a bit more feedback!

@lukewagner
Copy link
Member

Awesome, lgtm, thanks again! Once things are implemented enough to feel good about this PR, lmk and I'll merge this.

This commit addds some explanation of the intended usage pattern of
feature gates in order to make the transition points and functionality
easier to identify/reason about.

Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
@vados-cosmonic
Copy link
Contributor Author

Yup! I'll squash here and see what I can do about updating implementation in other spots!

vados-cosmonic added a commit to vados-cosmonic/wasm-tools that referenced this pull request Aug 26, 2024
This commit removes the optional `feature` specification from feature
gates (`@since`, in particular).

This change should simplify the usage of feature gates (see
WebAssembly/component-model#387) for more
discussion.

This is a breaking change for those who were depending on `wit-parser`
as `feature` now no longer present.

Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
vados-cosmonic added a commit to vados-cosmonic/wasm-tools that referenced this pull request Aug 26, 2024
This commit removes the optional `feature` specification from feature
gates (`@since`, in particular).

This change should simplify the usage of feature gates (see
WebAssembly/component-model#387) for more
discussion.

This is a breaking change for those who were depending on `wit-parser`
as `feature` now no longer present.

Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
github-merge-queue bot pushed a commit to bytecodealliance/wasm-tools that referenced this pull request Aug 26, 2024
…#1741)

* Remove feature flag from post-stabilization feature gates

This commit removes the optional `feature` specification from feature
gates (`@since`, in particular).

This change should simplify the usage of feature gates (see
WebAssembly/component-model#387) for more
discussion.

This is a breaking change for those who were depending on `wit-parser`
as `feature` now no longer present.

Signed-off-by: Victor Adossi <vadossi@cosmonic.com>

* Update wit-component to not use feature option in since

This commit updates `wit-component` to remove reliance on the
`feature` option when dealing with post-stabilization (`@since`)
feature gates.

Signed-off-by: Victor Adossi <vadossi@cosmonic.com>

* Update tests to remove optional feature on since gates

Signed-off-by: Victor Adossi <vadossi@cosmonic.com>

---------

Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
@vados-cosmonic
Copy link
Contributor Author

Hey @lukewagner so at this point the optional feature is removed (in a semi-breaking manner, those changes should percolate through but have rarely few people affected), anything else I'm missing? We don't have a new version of wit-parser and/or wit-component out yet of course, but it should be relatively real now going forward.

@lukewagner
Copy link
Member

Great, thanks again for all the work!

@lukewagner lukewagner merged commit 41bef2a into WebAssembly:main Aug 27, 2024
1 check passed
@vados-cosmonic vados-cosmonic deleted the explicit-feature-gate-scenarios branch August 27, 2024 15:41
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.

Clarify usage patterns for feature gates
5 participants