Reduce core dependencies

[ramsayleung]

Reducing core dependencies, check this issue #108 for more details:

• replacing rand with a lightweight replacement getrandom
• optional dotenv dependency
• optional browser dependency
• documentation

[marioortizmanero]

By the way, I don't seem to have permissions to edit this PR. That, or I don't know how to do it:

/tmp/rspotify reduce-core-dependencies ⇡ 7s
❯ git push
Username for 'https://github.com': marioortizmanero
Password for 'https://marioortizmanero@github.com': 
remote: Permission to ramsayleung/rspotify.git denied to marioortizmanero.
fatal: unable to access 'https://github.com/ramsayleung/rspotify.git/': The requested URL returned error: 403

I think I either have to be a collaborator, or I would have to create the PR, and then you edit it as the original creator.

[ramsayleung]

By the way, I don't seem to have permissions to edit this PR. That, or I don't know how to do it:

/tmp/rspotify reduce-core-dependencies ⇡ 7s
❯ git push
Username for 'https://github.com': marioortizmanero
Password for 'https://marioortizmanero@github.com': 
remote: Permission to ramsayleung/rspotify.git denied to marioortizmanero.
fatal: unable to access 'https://github.com/ramsayleung/rspotify.git/': The requested URL returned error: 403

I think I either have to be a collaborator, or I would have to create the PR, and then you edit it as the original creator.

My bad, I have invited you as a collaborator.

[marioortizmanero]

Ok so I've made both of these optional features. The thing is, should any of them be included by default? I don't see env-file being default, but browser could. Thoughts? It could still be disabled with default-features = false, which would make compile times ~2 seconds faster.

A quick way to know what people use to authenticate is by checking GitHub repos:

• rspotify:: returns 360 results.
• rspotify::get_token returns 244 results.
• rspotify::request_token returns 6 results.

Thus, more or less the 250/360 (69.4%) of the userbase needs the browser feature. Not sure.

[ramsayleung]

The thing is, should any of them be included by default? I don't see env-file being default, but browser could. Thoughts?

Agree, I think the browser feature should be included by default for which rspotify has no other way to help users get their credentials.

    use std::io;
    match spotify_oauth.get_cached_token() {
        Some(token_info) => Some(token_info),
        None => {
            request_token(spotify_oauth);
            println!("Enter the URL you were redirected to: ");
            let mut input = String::new();
            match io::stdin().read_line(&mut input) {
                Ok(_) => process_token(spotify_oauth, &mut input),
                Err(_) => None,
            }
        }
    }

When there is no cache token exists, e.g spotify_oauth.get_cached_token() return false, the only way to authenticate is pulling up a browser. If we have another way to authenticate without pulling up a browser, we could make browser feature optional, even remove this feature.

[ramsayleung]

Besides, of reducing core dependencies, I also plan to reduce some examples file. All of these files come from a same template, the only difference between them is that they calls different function with different day, everything else is same. I just use example file to test endpoint, but now I think maintain tons of example files is painful, so I prefer to remove some of them to keep this repository clear and maintainable.

[marioortizmanero]

Agreed! The examples are quite repetitive and don't offer much help for real projects. I would add more advanced examples instead of lots of very simple ones.

You can open a new issue or PR and I'll help you with that if you want.

[ramsayleung]

I just open a new PR to reduce examples #113, feel free to contribute to it.
PS: I think we should merged the PR of reducing examples before this PR, so we don't have to fix those check errors detected by CI.

[marioortizmanero]

PS: I think we should merged the PR of reducing examples before this PR, so we don't have to fix those check errors detected by CI.

Shouldn't we work on #95 first then? Did you figure something out? The error thread 'test_artist_top_tracks' panicked at 'dispatch dropped without returning error' is quite cryptic, maybe it could also be improved for future users that see it. This issue seems to point out that it's happening because our client is global: hyperium/hyper#2112

[ramsayleung]

Shouldn't we work on #95 first then? Did you figure something out? The error thread 'test_artist_top_tracks' panicked at 'dispatch dropped without returning error' is quite cryptic, maybe it could also be improved for future users that see it. This issue seems to point out that it's happening because our client is global: hyperium/hyper#2112

Yes, we should. The problem of tests is that the current test cases mostly depend on Spotify's API, the test case would actually send a request to Spotify's server. So the failed tests are pretty mysterious, it's not only the test_artist_top_tracks case, but also other cases:
https://github.com/ramsayleung/rspotify/runs/985549966

My intuition about fixing failed tests is just running them more times, for example, put a test case into a loop, and run it repeatedly(e.g 10 times), if any of them passes, the test passes. But it perhaps triggers the rate-limit strategy of Spotify API

[marioortizmanero]

Are you sure it's because of the Spotify rate limits anyway? Other Spotify API libraries like tekore have a 100% code coverage, meaning that they have even more tests than us, yet I don't recall them dealing with these issues. According to the issue I linked in the previous comment, it might have to do with hyper's client.

[ramsayleung]

Are you sure it's because of the Spotify rate limits anyway?

To be honest, not sure, just my worry.

According to the issue I linked in the previous comment, it might have to do with hyper's client.

No clue about this issue.

[marioortizmanero]

I think this PR is ready to be merged now as well. Check out the changelog and documentation I wrote and let me know if you'd change anything.

I also fixed a typo in user_playlist_remove_specific_occurrenes_of_tracks that I saw while reading code.

Can you get rid of the GitGuardian security check? You have to log into GitGuardian as the owner of the repo and disable it.

[ramsayleung]

Merged :)