Skip to content

Move EvaluateArguments context variables to end of enums#346

Merged
connor4312 merged 1 commit intomicrosoft:mainfrom
WardenGnaw:dev/waan/fixContextOrdering
Nov 8, 2022
Merged

Move EvaluateArguments context variables to end of enums#346
connor4312 merged 1 commit intomicrosoft:mainfrom
WardenGnaw:dev/waan/fixContextOrdering

Conversation

@WardenGnaw
Copy link
Member

This PR moves the newly added 'variables' enum for EvaluateArguments context from 87fdf65 to be added at the end instead of at the start of the array.

Reordering the parameters is breaking change for downstream generators.

cc @andrewcrawley

This PR moves the newly added 'variables' enum for EvaluateArguments
context to be added at the end instead of at the start of the array.

Reordering the parameters is breaking change for downstream generators.
@WardenGnaw WardenGnaw changed the title Move EvaluateArguments context to end of enums Move EvaluateArguments context variables to end of enums Nov 4, 2022
@roblourens
Copy link
Member

roblourens commented Nov 4, 2022

This seems fine but can you tell me more about what it breaks?

@andrewcrawley
Copy link

The C# code generator that we own generates strongly typed enums to represent the enums in the protocol. Adding a new item to one of these enums other than at the end causes the underlying values of the previous enum values to change, that is:

enum ContextValue {
    Watch = 0,
    Repl = 1,
    // ...
}

becomes

enum ContextValue {
    Variables = 0
    Watch = 1,
    Repl = 2,
    // ...
}

I don't think this is an issue for the TypeScript generator, since it uses union types for this instead of enums, but it breaks backwards compatibility between different versions of our protocol library.

@connor4312
Copy link
Member

connor4312 commented Nov 8, 2022

I think depending on the order of enum types, which are effectively a Set, is a little questionable, but I'm fine taking this fix

@connor4312 connor4312 merged commit 580c34d into microsoft:main Nov 8, 2022
@connor4312 connor4312 added this to the November 2022 milestone Nov 8, 2022
@WardenGnaw WardenGnaw deleted the dev/waan/fixContextOrdering branch November 15, 2022 21:22
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.

4 participants