[PM-14235] Add more docs to bitwarden_vault #102
Conversation
| } | ||
|
|
||
| /// An extension trait for the [VaultClient] struct to provide vault functionality. | ||
| pub trait VaultClientExt<'a> { |
There was a problem hiding this comment.
I'm not exactly sure what these "extensions" are or will be used for.
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
=======================================
Coverage 65.24% 65.24%
=======================================
Files 181 181
Lines 13821 13821
=======================================
Hits 9018 9018
Misses 4803 4803 ☔ View full report in Codecov by Sentry. |
|
|
||
| use crate::VaultParseError; | ||
|
|
||
| /// Encrypted Collection state |
There was a problem hiding this comment.
What does state imply?
| /// Encrypted Collection state | |
| /// Encrypted Collection |
| pub manage: bool, | ||
| } | ||
|
|
||
| /// Decrypted Collection state |
There was a problem hiding this comment.
| /// Decrypted Collection state | |
| /// Decrypted Collection |
| } | ||
|
|
||
| impl LocateKey for Collection { | ||
| /// Returns a [SymmetricCryptoKey] on success, [CryptoError] on failure |
There was a problem hiding this comment.
This comment conveys the exact same information as the type interface. We generally want to avoid these type of comments since they can easily get out of sync with the actual code. We're also actively refactoring these logic.
|
|
||
| #[derive(Serialize, Deserialize, Debug, JsonSchema)] | ||
| #[serde(rename_all = "camelCase", deny_unknown_fields)] | ||
| /// Data returned from `/accounts/profile`. |
There was a problem hiding this comment.
This is false. ProfileResponseModel from bitwarden-api-api is the response from the endpoint. This is a part of the SyncResponse which is the internal SDK response from performing a sync.
|
|
||
| #[derive(Serialize, Deserialize, Debug, JsonSchema)] | ||
| #[serde(rename_all = "camelCase", deny_unknown_fields)] | ||
| /// Data returned from `/sync`. |
There was a problem hiding this comment.
This is false. SyncResponseModel from bitwarden-api-api is the response from the endpoint. This is an internal model containing the data returned by calling sync().
| /// let vault = VaultClient::new(&client); | ||
| /// ``` | ||
| pub struct VaultClient<'a> { | ||
| /// A vault client. |
There was a problem hiding this comment.
This is not a vault client though. This is the root SDK client.
| /// A vault client. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ```rust | ||
| /// use bitwarden_core::{Client,ClientSettings,DeviceType}; | ||
| /// use bitwarden_vault::VaultClient; | ||
| /// | ||
| /// let client = Client::new(Some(ClientSettings { | ||
| /// identity_url: "https://identity.bitwarden.com".to_owned(), | ||
| /// api_url: "https://api.bitwarden.com".to_owned(), | ||
| /// user_agent: "Bitwarden Rust-SDK".to_owned(), | ||
| /// device_type: DeviceType::ChromeBrowser, | ||
| /// })); | ||
| /// | ||
| /// let vault = VaultClient::new(&client); | ||
| /// ``` |
There was a problem hiding this comment.
We don't want people to manually. wire up VaultClient's and you should generally use the Extension i.e. client.vault() -> VaultClient.
There was a problem hiding this comment.
That sounds reasonable yeah. I looked into it and VaultClient is the only partial client constructor that is pub too, the rest are private.
|
|
||
| /// Syncs the [VaultClient] with the server. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ```rust | ||
| /// # use bitwarden_core::{Client,ClientSettings,DeviceType}; | ||
| /// # use bitwarden_vault::{VaultClient,SyncRequest, SyncResponse}; | ||
| /// async fn sync_vault(client: &Client) { | ||
| /// let vault = VaultClient::new(client); | ||
| /// let request = SyncRequest { | ||
| /// exclude_subdomains: Some(false), | ||
| /// }; | ||
| /// | ||
| /// let result = vault.sync(&request).await; | ||
| /// match result { | ||
| /// Ok(response) => println!("Response: {:?}", response), | ||
| /// Err(error) => { | ||
| /// eprintln!("Sync failed: {:?}", error); | ||
| /// }, | ||
| /// } | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
We probably don't actually want to expose this as public since it's to my knowledge unused.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-14235
📔 Objective
A first attempt at documenting stuff in
bitwarden_vault. I'm missing context for a lot of the why on things, so most of it is documenting the what and adding examples where I can. Admittedly, explaining the what probably isn't as useful, so I'd appreciate some pointers on how to make these docs better.⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes