capabilities: implement ping#30
Conversation
…raw self-convention)
malone-at-work
left a comment
malone-at-work
left a comment
There was a problem hiding this comment.
Looks Good - one suggestion.
| villagesql::func!(ping_impl, "ping", [] -> villagesql::Type::Int), | ||
| ], | ||
| capabilities: [ | ||
| PingCapability::request(), |
There was a problem hiding this comment.
request() does't sound like the right name; could we instead create a Capability trait with that function? and rename capablities, to requires: so registration would look something like:
requires: [
PingCapabilty,
]
There was a problem hiding this comment.
Hey Mike, went ahead and implemented what we talked about. Added a Capability trait and renamed capabilities: to requires:.
Tried my best to match with what already exists on the C++ side. The vtable is now a field inside each capability instance instead of a global static. The authors now will declare the capability as a static and pass a reference to it:
static PING: PingCapability = PingCapability::new();
// ...
requires: [
&PING,
]
Now I'm moving on to apply the same type of changes to the sys_var branch.
|
One other suggestion can you do a PR description like: "capabilities: implement Ping" |
shelby-fulton-villageSQL
changed the title
| /// monotonically incrementing counter. Returns NULL if the capability was not | ||
| /// populated (e.g. `vsql_allow_preview_extensions` is OFF). | ||
| #[allow(clippy::cast_possible_wrap)] | ||
| fn ping_impl(_args: &[InValue]) -> VdfReturn { |
There was a problem hiding this comment.
Sorry - one more nit - this maybe should be 'call_ping_impl()' and the VDF 'call_ping'? I know you are basing it on the C++, but it is confusing. This calls ping, which is implemented by the server.
There was a problem hiding this comment.
Made those changes if you want to check before I squash and merge
3 participants
Capabilities were ported over and ping was implemented. Claude generated most of this. Learning more Rust before continuing with the rest of the capabilities.