-
Notifications
You must be signed in to change notification settings - Fork 225
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 madeRequired decorator and tests #3292
Conversation
@microsoft-github-policy-service agree |
❌ There is undocummented changes. Run The following packages have changes but are not documented.
Show changes |
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 a lot for the contribution, from the pure decorator point of view this looks good.
However this current implementation will mot allow a property go go optional -> required -> optional
multiple times(or the other way around).
This is something we can do today with @added
and @removed
.
What we basically need is to create the same availability timeline but for the requireness of a property and have a isOptionalAtVersion
helper.
You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/3292/ Check the website changes at https://tspwebsitepr.z22.web.core.windows.net/prs/3292/ |
Not sure I can work out all of that from an outside view. I understand the problem but not enough of the semantics of how this all works just yet. I'll have a look and see what I can reason about given what the added and removed decorators do. |
Also what's with the "undocumented changes"? It doesn't seem to show me any changes to document.... |
Yeah not worries, it was scheduled to be worked on this coming month so I can add that to your pr. For the doc change it's asking for change log tracking. Just click on the link in the comment to add a change entry. Or use Pnpm change add locally |
/azp run typespec - pr tools |
Azure Pipelines successfully started running 1 pipeline(s). |
I thought I had this down but the PR checks apparently have ME down instead :p |
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.
@borrrden talked with the team and we'll file a follow up issue to add the scenarios I was talking about above as you already cover the 99% use case.
Hopefully I got all the places I needed to get and added enough tests. My methodology was pretty much "Look at what madeOptional does, and do the opposite"
Closes #2731