Skip to content
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

Ungenericify Deepgram struct #49

Closed
algesten opened this issue Aug 3, 2023 · 1 comment
Closed

Ungenericify Deepgram struct #49

algesten opened this issue Aug 3, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@algesten
Copy link
Contributor

algesten commented Aug 3, 2023

Proposed changes

The Deepgram<K> struct is generic over the API key K. The signature is:

pub struct Deepgram<K>
where
    K: AsRef<str>,

I propose K is cloned and stored internally in a String.

Context

There is no obvious benefit to the API user to be generic over an API key. I can see two reasons to do this, and neither seem to hold up.

  1. Save space. However it's 40 bytes as a string (and looks like a hexadecimal number, so I assume it can be stored as a number too?). It seems to be a very small gain.
  2. Allow user to control storage of the API key. In a security paranoid environment maybe it would be possible to hold the key in a "secure enclave" or similar tech. Though AsRef would defeat this, since a simple operation is supposed to release the key. If I don't want the Deepgram struct to remain in memory with the key, I can simply drop the struct. If the API want to ensure the key is zeroed out from memory, the API can impl Drop for Deepgram and zero the memory where the key was stored.

Possible Implementation

 fn new(key: impl AsRef<str>) -> Self {
    let api_key = key.as_ref().to_owned();
    Self { api_key }
 }
@algesten algesten added the enhancement New feature or request label Aug 3, 2023
andrewhalle added a commit that referenced this issue Aug 4, 2023
The `Deepgram` struct is unnecessarily generic. Storing the API key as a
generic offers flexibility which probably isn't warranted by any use
cases.

Fixes #49
@lukeocodes
Copy link

hey thanks for this @algesten we'll try and review it asap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants