-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Draft proposal for experimental field #2386
Conversation
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.
Thanks for the PR and making it through the proposals template! Just some thoughts.
proposals/005_Experimental.md
Outdated
A new boolean field named `experimental`, defaulting to `experimental`, is added to: | ||
|
||
- Operation | ||
- Schema |
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.
I wonder whether adding the experimental
field to higher level objects (such as parameter
, requestBody
and response
) and dropping it from schema
would work better? It might resolve the 'unanswered question' about experimental schema use in non-experimental operations.
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.
I'd missed parameter - have added that now, with a qualifier that it couldn't be used with with in: path
required: true
since adding a new required parameter is a breaking change.
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.
I see your point about schema
, I agree removing it from there would simplify. I'm not sure I understand how putting on requestBody
and response
instead would alleviate though, could you elaborate?
One use case I've had come up is having a subset of an object be experimental, like "we've added some more detail to this response, we're not totally sure about it yet but the rest of it remains stable in the meantime", so it would be a shame not to cover that, but perhaps a leaner way to introduce the concept initially?
@MikeRalphson thanks for the feedback and apologies for taking a while to follow up! I've made some edits and responded inline. |
As a result of #2604 the filename for this proposal should change to use a dated prefix. Please could you update this PR to match? You can use the original PR date. Apologies for the extra work. |
@MikeRalphson no problem, that's done. |
Since this PR was submitted, we've created some special interest groups (SIGs), and one of them is planning to address the lifecycle, which I think this falls into. There isn't a lot of traction yet, and we're still working out how the group will actually operate, but I'd also suggest we use Discussions, either in the spec repo (this one) or possibly around the new repo linked above. There are some similarities to #1973 worth noting, as well. (Thanks @darrelmiller for flagging this proposal to the nascent lifecycle SIG!) |
Thanks @earth2marsh, that looks interesting. What would be the right next step for this PR? |
TBH, I'm not sure. I think as the lifecycle SIG gets traction, that this would be the basis of a discussion there where the overall lifecycle issues are considered? I'm not sure anyone has stepped up to run it yet, but I plan to stay involved and will flag this (and reach out) once it begins getting momentum. |
Filed #3257 to revive and clarify our proposals process. Once we figure it out, we'll resolve this PR accordingly. |
@OAI/tsc is this proposal, like PR #3286, better started as an If so, it would seem like this document should be associated with that, rather than a proposal in this repo. If not, we will get to looking at the draft proposal process soon and can evaluate this once we've revived that process. |
This is already "out there" as an extension (citation: https://github.com/search?q=%22x-experimental%3A+true%22&type=code), and there was quite a bit of engagement around the issues for it. Can we say what is needed for this to merit consideration for 3.2? |
@lornajane oh nice! It... did not even occur to me to check 😅 🤦 Yeah, that seems like a good reason to at least accept the proposal into the repo (meaning merging this PR, assuming it is in alignment with the |
For what it's worth, we use |
The Reports of an API Lifecycle SIG have been greatly exaggerated, it looks like there's a repo and a slack channel but no activity since the setup a couple of years ago in either. |
@davidjgoss Would you be able to revisit this proposal and make some of the proposed updates? I'm in support of us adopting this change. |
@lornajane sure, I’ll try and get those changes done over the next few days or so. (Thanks everyone for input.) |
Co-authored-by: Ralf Handl <[email protected]>
Just noting the |
@@ -113,12 +113,13 @@ This kind of requirement is handled for TypeScript libraries by [api-extractor]( | |||
### Unanswered Questions | |||
|
|||
- If an operation is not marked as experimental, but it is using a schema which is (i.e. as its request object), then it is implicitly also unstable. Would this usage be considered invalid? |
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.
In the request object example only requests using an experimental feature are potentially unstable, so I'd see this as a valid combination.
|
||
## Backwards compatibility | ||
|
||
The `experimental` field would default to false, meaning existing behaviour is preserved, and the new field is only used on an opt-in basis. | ||
|
||
`experimental` can coexist with `deprecated` - an operation, parameter or schema can be both experimental and deprecated, having never gotten to a stable point before being deprecated. |
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.
Not sure whether I'd bother with deprecating an experimental feature, the Motivation section states that experimental aspects can
just get removed
and the Proposed solution section repeats that it
may change or be removed without a major version bump
What would it mean to deprecate an experimental feature as opposed to not removing it? That it will still work in this major version and disappear with the next major version bump?
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.
@adamaltman you had the opposite view in another comment, what do you think?
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.
I think this is a solid proposal that adds something that we do see used "in the wild". I'm approving and accepting this pull request, the changes can be added n 3.1 or any later minor version.
Thanks @lornajane. What would be the next step for this, doing a PR against the spec itself? |
(Not sure if this is the best place/right way to start this conversation, but would be interested in feedback.)
TLDR: a way to include items in an API but marked as "experimental" to set expectations that they could change or go away, and that wouldn't be a "breaking change" in semver terms.