Conversation
Deploying contributing-docs with
|
| Latest commit: |
250d8b5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://05dfce03.contributing-docs.pages.dev |
| Branch Preview URL: | https://platform-document-client-str.contributing-docs.pages.dev |
|
Great job! No new security vulnerabilities introduced in this pull request |
|
|
||
| :::warning | ||
|
|
||
| Avoid the thin passthrough pattern, where the client delegates to free functions defined elsewhere. |
There was a problem hiding this comment.
suggestion:
| Avoid the thin passthrough pattern, where the client delegates to free functions defined elsewhere. | |
| Avoid the thin passthrough pattern, where the client delegates to pure functions defined elsewhere. |
There was a problem hiding this comment.
I feel like calling them pure wouldn't be entirely correct, as some of these functions will modify the client's state in some way (think of initialize_user_crypto for example)
There was a problem hiding this comment.
Ah true, hmm, apparently this is just me being a noob, "free function" is an established term :)
There was a problem hiding this comment.
I relied on Claude for this part, as I wasn't sure what to call them either, especially in Rust. Would it be clearer to just say "to methods defined elsewhere"?
There was a problem hiding this comment.
I think the warning text is clear enough, though I agree that the free/associated function terminology might not be clear for newcomers. Maybe the easiest way would be to include a simple example of what not to do?
impl LoginClient {
// Avoid delegating the entire implementation to another function like this.
pub async fn login_with_password(&self, data: LoginData) -> Result<()> {
login_with_password(self.client, data).await
}
}
coroiu
left a comment
coroiu
left a comment
There was a problem hiding this comment.
I agree with Danis suggestion with the example, all good otherwise
| ├── mod.rs | ||
| └── method_name/ | ||
| ├── mod.rs | ||
| ├── method_name_impl.rs # impl DomainClient { fn method_name() } and tests |
There was a problem hiding this comment.
I don't think it's recommended to name files _impl.
| ├── method_name_impl.rs # impl DomainClient { fn method_name() } and tests | |
| ├── method_name.rs # impl DomainClient { fn method_name() } and tests |
5 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-30764
📔 Objective
Capture recommended patterns for creating
Clientimplementations insdk-internal.Creating a "client" will be required for any new feature crate, so having best practices for how this code is structured feels important. We have several different patterns currently in play in
sdk-internal, and this PR attempts to guide future development toward our current best practices and allow us to reference this documentation for guidance.