-
Notifications
You must be signed in to change notification settings - Fork 313
[Storage] Flattened clients v2 #3088
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
[Storage] Flattened clients v2 #3088
Conversation
} | ||
|
||
impl GeneratedBlobClient { | ||
fn from_url( |
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 may be able to use #[tracing::new]
here - it should work and hide the tracer initialization.
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.
Thanks Larry, that works out much cleaner! However, this requires us to make the from_url
function on the underlying GeneratedBlobClient
now public (which is not desired as everything should go through our convenience layer).
This is not yet the final design, so Storage will discuss internally and ensure we land on our final design before requesting any Core code changes (i.e. removing the requirement here that the visibility must be public to utilize the macro)
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 might want to resync with main (which may be painful because of Heath's recent changes). He ran into the same issue and we decided that the public requirement wasn't necessary or appropriate.
So changing core is probably the right thing to do.
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.
@LarryOsterman Following up on this, was the change to remove the public requirement live yet or still living in an unmerged PR? I am rehydrating this after the Core changes and still facing the error:
Thanks!
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.
Unfortunately it appears it's still in a private branch. If you'd like, you can certainly fix this yourself - it's lines 376 and 377 of tracing_new.rs
in the azure_core_macros package.
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.
Ah gotcha. No worries, we are still ironing out some kinks in the Storage side of the design, but I can certainly contribute that back if it continues to be a problem for us. Thanks Larry 😄
.tsp
: Azure/azure-rest-api-specs#37830