-
Notifications
You must be signed in to change notification settings - Fork 23
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
Prepare for Eventual Protocol Migrations. #1898
Conversation
build.gradle.kts
Outdated
if (!isForceProtocolCopy) { | ||
return | ||
} | ||
val fromEnvironmentVariable = System.getenv("CODY_DIR") |
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.
You can use downloadCody()
instead.
It uses cody from CODY_DIR
if defined, if not then downloads cody specified by cody.commit (or uses one from cache, if exists).
This way it will also work without specifying CODY_DIR
.
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 way when we will have a class with exactly the same name it will be clear it is needed because the one automatically generated is somehow incorrect (or, for the time being, not migrated yet).
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.
Good point! 👍
|
||
import com.sourcegraph.cody.agent.protocol_generated.ClientInfo as ProtocolClientInfo | ||
|
||
data class ClientInfo( |
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.
@RXminuS I think it is a bit confusing that helper classes have the same name as the generated one. I feel like a bit idiomatic for this use case would be to simply have a construction method in the protocol_generated.ClientInfo
extension.
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.
Everything in protocl_generated is not allowed to be modified and also contains headers to warn against doing so. If we are missing helpers that we can make an issue to extend the generation system.
But I understand what you mean about the naming duplication being confusing. That should mostly go away though as we either use the new protocol classes directly. And for the other 10% of cases I think we're probably best going with a Factory design pattern instead of modifications to classes or wrapper classes.
cc43ed1
to
9bf3c0c
Compare
4549dd9
to
caaeec9
Compare
Some parts of the Agent send a MAX_INT which in Node is a "Long"
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'm very excited about this! I hit on a bug yesterday in Eclipse where deserializing WebviewMessage
crashed because the bindings expect a string for a URI
where a JSON serialized object appears. I'm hoping to not have to deal with these issues in WebviewMessage
because we're only forwarding string-encoded messages for 99% of the features
val newContent = | ||
""" | ||
|/* | ||
| * Generated file - DO NOT EDIT MANUALLY |
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.
👍🏻
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.
We can btw add a similar comment to the top of the file in the code generator. My thinking was that we can just do cp -r ...
in a bash script and ideally avoid custom gradle tasks like this one
.replace(": Int", ": Long") | ||
// replacing classpath | ||
.replace( | ||
"com.sourcegraph.cody.protocol_generated", |
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.
Let's remove these workarounds, it only takes a couple minutes to incorporate these changes in the codegen sourcegraph/cody#4915
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 just wanted to have a single PR ready to ship the feature as it's time critical. But there's some time left so let's do so.
I might just want to add so we always generate an empty companion object too as it makes extending easier and I can get rid of the factory classes
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.
The bindings have been unadopted for ~5 months, is it really time critical?
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.
haha; No not in that sense. But my use of the auto bindings in a time critical feature just because I didn't want to dig a deeper hole meant I tried to be cautious about poking at adjacent stuff I didn't fully understand the impact of.
But you have showed me the way. And properly fixing this in codegen is a lot easier than I thought 😊
(This is a stacked PR)
Superseded by #1921 |
Fixes CODY-2830
At the moment we rely on manually written protocol messages and server/client implementations. This is a first step towards making sure that we can migrate towards auto generated bindings.
More importantly this was done now because for the "Fix It" feature a few new protocols need to be implemented. So rather than digging a deeper hole this now allows to implement those in a way that will make migrating easier once all the other messages have been converted as well.
So to summarize, this isn't so much about migrating everything now; rather making sure that everything new added doesn't make the work to migrate larger. I created a placeholder project for it so I can collect issues for the actual migration there.
For now I've only "migrated" a few low-hanging messages that were easy to do more as an example than anything else. No functionality should have changed. Couldn't quite verify though given the e2e test flake. Also added a simple helper to make working with Enums a bit nicer.
Right now there's no rigid testing / verification yet that the copied protocols are of the exact version mentioned etc. Instead there's a ad-hoc task/script that you can run with
./scripts/copy-protocol.sh
that will copy the protocols from CODY_DIR (or by default<git_root>/../cody/
)Test plan