-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
[EG] union updates #32560
base: main
Are you sure you want to change the base?
[EG] union updates #32560
Conversation
Next Steps to MergeNext steps that must be taken to merge this PR:
|
Generated ApiView
|
@@ -1114,6 +1114,10 @@ union CommunicationIdentifierModelKind { | |||
/** Microsoft Teams User */ | |||
"microsoftTeamsUser", | |||
|
|||
/** Microsoft Teams Application */ | |||
"microsoftTeamsApp", |
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.
azure-rest-api-specs/specification/communication/data-plane/Common/stable/2023-11-15/common.json
Line 81 in dc6126a
"microsoftTeamsApp" |
API change check APIView has identified API level changes in this PR and created following API reviews. |
|
||
/** Delivered */ | ||
"delivered", | ||
Delivered: "delivered", |
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.
Why did we add the keys here but remove in the other model?
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.
Added the keys here b/c in this union I got twisted around on the swagger def here:
. The on the wire enum has uppercase, but values in x-ms-enum has lowercase@@ -867,10 +867,10 @@ union AcsRouterLabelOperator { | |||
/** Router Worker Selector State */ | |||
union AcsRouterWorkerSelectorState { | |||
/** Router Worker Selector State Active */ | |||
Active: "active", | |||
"active", |
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.
"active", | |
active: "active", |
Should we be explicit? Sure it's redondant, but cleaner?
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.
Yes I can do the same to all the unions
|
||
/** Router Worker Selector State Expired */ | ||
Expired: "expired", | ||
expired: "expired", |
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.
this can go back to being capital keys
|
||
string, | ||
} | ||
|
||
/** Router Worker Selector State */ | ||
union AcsRouterWorkerSelectorState { | ||
/** Router Worker Selector State Active */ | ||
Active: "active", | ||
active: "active", |
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.
this is capitalized by JS in GA
Hi, @l0lawrence. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
Checked all
union
types in system event typespec and ensured accurate key/value pairing capitalization. Found a missing value under acs