Reducing rspotify's core dependencies #108
Comments
It's a great suggestion, which makes it more easier to build automatic tests without
Yes, I originally use
Honestly, it's the only usage of
If it's the best choice for us, why not? |
My point is that opening a new browser window/tab is only needed for those creating CLI applications:
As long as it's well documented I think it will be fine.
Great. I'll try to come up with something for that and I'll open a new PR once I have it ready. Do you want me to create these features in |
I prefer to open a PR to implement this reduction, we both could commit to this PR. And now I am trying to replace |
I just noticed
So I don't think removing |
|
I just create a code snippet with use getrandom::getrandom;
static ALPHANUM: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
fn main() {
let random_string = generate_random_string(16);
println!("{:?}", random_string);
}
fn generate_random_string(length: usize) -> String {
let mut buf = vec![0u8; length];
if let Err(why) = getrandom(&mut buf) {
panic!("Fail {:?}", why)
};
let length = ALPHANUM.len();
let mut random_string = String::from("");
for byte in buf.iter() {
let c = ALPHANUM.as_bytes()[*byte as usize % length] as char;
random_string.push(c);
}
random_string
}it works. in
|
|
Looks great! Here's what I came up with: use getrandom::getrandom;
fn main() {
let random_string = generate_random_string(16);
println!("{:?}", random_string);
}
fn generate_random_string(length: usize) -> String {
static ALPHANUM: &[u8] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789".as_bytes();
let mut buf = vec![0u8; length];
getrandom(&mut buf).unwrap();
let range = ALPHANUM.len();
return buf.iter().map(|byte| {
ALPHANUM[*byte as usize % range] as char
}).collect()
}Iterators are cool! :)
|
|
Here's a comparison I've made:
use rand::distributions::Alphanumeric;
use rand::{self, Rng};
fn main() {
for _ in 1..100000 {
let random_string = generate_random_string(16);
println!("{:?}", random_string);
}
}
fn generate_random_string(length: usize) -> String {
rand::thread_rng()
.sample_iter(&Alphanumeric)
.take(length)
.collect()
}
use getrandom::getrandom;
fn main() {
for _ in 1..100000 {
let random_string = generate_random_string(16);
println!("{:?}", random_string);
}
}
fn generate_random_string(length: usize) -> String {
static ALPHANUM: &[u8] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789".as_bytes();
let mut buf = vec![0u8; length];
getrandom(&mut buf).unwrap();
let range = ALPHANUM.len();
return buf.iter().map(|byte| {
ALPHANUM[*byte as usize % range] as char
}).collect()
}use std::time::{SystemTime, UNIX_EPOCH};
use oorandom::Rand32;
fn main() {
for _ in 1..100000 {
let random_string = generate_random_string(16);
println!("{:?}", random_string);
}
}
fn generate_random_string(length: usize) -> String {
// Using a deterministic seed such as the Unix timestamp. For a non
// deterministic seed, the `getrandom` crate would have to be used, which
// makes no sense because it can be used directly to generate the random
// numbers.
//
// If this method were to be used more than once per second, the results
// would also be identical. This could be fixed by using `.as_nanos` instead
// and converting to `u64`, which should have a similar performance.
//
// In conclusion, using oorandom doesn't make sense in this case at all,
// but I'll benchmark it just out of curiosity.
let seed = SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap()
.as_secs();
static ALPHANUM: &[u8] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789".as_bytes();
let mut rng = Rand32::new(seed);
let mut buf = String::with_capacity(length);
let range = ALPHANUM.len();
for _ in 0..length {
buf.push(ALPHANUM[rng.rand_u32() as usize % range] as char)
}
buf
}
The |
|
Cool, I just open a PR #110 to reduce core dependencies, feel free to contribute to this.
Just out of curiosity, would you like to add |
Sure, I've added a benchmark with |
|
This can be closed now that #110 was merged. |
I've been creating various PRs to clean up some unnecessary dependencies I found and such. Not only does this decrease compilation times, but this may also reduce binary size in case the optimizer is unable to know a module isn't used at all.
Here are some of my proposals, don't hesitate on criticizing them; I want to discuss these changes with @ramsayleung and if possible with more users of rspotify. It might also be too much to remove some of them:
Optional features
webbrowseroptional, under someclifeature. This dependency isn't used by all the users, since some may not need to open a browser for the authentication process. It's only used for theutil::request_tokenfunction currently.dotenvoptional, under someenvfeature. This is also only used by some users, mostly web servers.These changes will break backward compatibility, so a new version would have to be released. I could create an
UPGRADING.mdfile that explains what changes were made in order to make this process easier.Removing dependencies
cargo run --example current_playingto 177, so ~14 less dependencies after Upgraded dependencies #105lazy_staticis only used inclient.rsforCLIENThere. ThisCLIENTvariable is then only used once here. I haven't usedlazy_staticmyself, but I wanted to re-evaluate the following questions:- Is it necessary that
CLIENTis a global, static, public variable?- Is
lazy_staticis necessary here?randnecessary only forutil::generate_random_stringhere?randis a considerably big dependency because it includes lots of things that are often unnecessary. I recently saw this PR Replace rand with oorandom rust-lang/rust-analyzer#5574 that replaced it withoorandom, a much simpler and straightforward crate. Could this also be applied to this use case?The text was updated successfully, but these errors were encountered: