fix(codegen): Adds special cases for AuthStatus to re-enable codegen
#5854
Conversation
| const unionTypes = this.unionTypes(type) | ||
| // HACK: We have to special case the `AuthStatus` type because it's | ||
| // SCIP doesn't recognize the `authenticated` member as a constant type | ||
| // and thus picks the wrong discriminator | ||
| if (info.display_name === 'AuthStatus') { | ||
| return { | ||
| symbol: info.symbol, | ||
| discriminatorDisplayName: 'authenticated', | ||
| members: unionTypes.map(type => ({ | ||
| type, | ||
| value: type.type_ref.symbol.includes('Authenticated'), | ||
| })), | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the actual hack for AuthStatus
There was a problem hiding this comment.
I am in love with the changes in this PR. Thank you @jamesmcnamara 🙏🏻 🙏🏻
The new language-specific classes clean up the code significantly. I'll share a bit more context via Slack
| // HACK: We have to special case the `AuthStatus` type because it's | ||
| // SCIP doesn't recognize the `authenticated` member as a constant type | ||
| // and thus picks the wrong discriminator | ||
| if (info.display_name === 'AuthStatus') { |
There was a problem hiding this comment.
This hack is OK for now. I think the cleanest fix for this is to introduce ProtocolAuthStatus with a string discriminator like in this PR here #5851
There's a limit how much we should reuse internal types in the protocol, and I think it's OK to draw the line at string-typed discriminators. It's not a lot of work to add dedicated protocol types so that we're more intentional about the design of the protocol. It's undesirable that small internal refactorings leak into diffs in the protocol that are irrelevant to agent clients.
There was a problem hiding this comment.
I like it! I incorporated your changes from that PR (I think I got all of them) and removed this hack. But I also left the code generation in place for generating deserializers based on booleans and integers. We'll probably never be able to use it, but if we do it it should just work 🤞
There was a problem hiding this comment.
Agreed on keeping Boolean and numeric discriminator since you e already done the hard work. The codegen logic in this tool is reaching a level of sophistication that's quite far beyond what most off-the-shelf tools support. Would be cool to turn it into a standalone OSS project one day :)
There was a problem hiding this comment.
Haha it's so true. I thought the codegen tool I wrote at my last company was pretty nifty but it's a toy compared to what this can do.
There was a problem hiding this comment.
Now we just need to convince #team-graph to accept adding scip.Signature sourcegraph/scip#231 and merging sourcegraph/scip-typescript#322 so we can stop running a fork of scip-typescript.
Then we need a nice API to read SCIP data from Sourcegraph and we're off to the races to generates all kinds of cool tooling/products 😎
sourcegraph#5854) Adds a ProtocolAuthStatus type to get around issues of using a boolean as a discriminator. Also refactors the code generation into separate classes for each language type and leaves all SCIP logic in Codegen. ## Test plan Regenerated all three language types and verified that code changes were just due to updates in the underlying types since disabling.
Adds a special case for the AuthStatus to fix the code generation issues. Also refactors the code generation into separate classes for each language type leaves all SCIP logic in Codegen.
Test plan
Regenerated all three language types and verified that code changes were just due to updates in the underlying types since disabling.
Changelog