-
Notifications
You must be signed in to change notification settings - Fork 5
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
Draft: add protos and generate code #4
base: main
Are you sure you want to change the base?
Conversation
386020e
to
ec4d751
Compare
ec4d751
to
59dee42
Compare
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.
🏅 looking good!! just some minor comments to address
Ok(BASE64_URL_SAFE_NO_PAD.encode(json_stamp.as_bytes())) | ||
} | ||
|
||
#[cfg(test)] |
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.
would be nice to retain some semblance of a test suite with the rewrite :)
Some(input_headers) => { | ||
for (_, value) in input_headers.iter() { | ||
if let Ok(val_header) = HeaderValue::try_from(value) { | ||
headers.insert("test keyname", val_header); |
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.
do we need this in the case that there are other headers the user would like to include? or purely for testing?
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.
cc @joshhchun
// TODO: Might need to check if path starts with "/", not sure how its called | ||
let mut url = format!("{}{}", self.base_url, path); | ||
|
||
// TODO: This can all be cleaned up/simplified with Url crate if needed |
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 not opposed to using a URL crate if it simplifies thing. any potential reason it'd be a bad idea?
If this is intended to be a general purpose SDK, I'd suggest that accessing environment variables directly (and the use of .env files) should be up to the SDK user, and not baked into the SDK itself. My personal use case is likely to see these values coming from other rust code. If you really want this for convenience, then env variable access should be optional, ie something like this this:
|
The code is based up existing stamping and request code in tkhq#4 and leverages the generated proto types
The code is based up existing stamping and request code in tkhq#4 and leverages the generated proto types
The code is based up existing stamping and request code in tkhq#4 and leverages the generated proto types
The code is based up existing stamping and request code in tkhq#4 and leverages the generated proto types
The code is based up existing stamping and request code in tkhq#4 and leverages the generated proto types
The code is based up existing stamping and request code in tkhq#4 and leverages the generated proto types
This PR is a draft for codegen in the Rust SDK: internally Turnkey uses proto3 to define all types and interfaces so it seems like a waste to force our users to manually write types. We can generate it all!
The
proto
folder now contains the source protos, andsrc/gen
contains generated code. I haven't yet tried to use this with a Rust project hence why this is still a draft PR.