Add unit tests for pure methods

[ramsayleung]

Description

Add unit tests for pure methods like get_uri and get_type methods without external dependencies

Motivation and Context

Add more unit tests for rspotify to make it solid and easy to maintain and refactor

Dependencies

None

Type of change

Add unit tests

How Has This Been Tested?

Check if all added tests pass

[ramsayleung]

For unknown reason, the test_read_token_cache test will fail occasionally. I am trying to figure out the reason.

[marioortizmanero]

Maybe we should do something to make the scopes easier to use? Perhaps a scopes! macro would be easier to use than the whole scope.split_whitespace().map(|x| x.to_owned()).collect() boilerplate: scope = scopes!("asd", "test").

In a different PR, of course, but I noticed while reading the changes in this one.

[ramsayleung]

Perhaps a scopes! macro would be easier to use than the whole scope.split_whitespace().map(|x| x.to_owned()).collect() boilerplate: scope = scopes!("asd", "test").

Great idea! Probably I should make such a scope! PR firstly, and then merge this PR into it.

[marioortizmanero]

We could just reuse a macro from a crate like maplit or velcro until rust-lang/rfcs#542 is worked on. Their implementation of hashset! isn't that complicated, but that way we could use stuff like hashmap! in the endpoints, which could come in quite handy.

It's an additional dependency but considering that it's something rust is working on to include in the future we might be able to eventually remove it. But it's a RFC still so give it at least a few months.

[ramsayleung]

I just add a macro named hashset! in this commit: faf6543 (#189) instead of an additional PR.

We could just reuse a macro from a crate like maplit or velcro until rust-lang/rfcs#542 is worked on

I personally prefer to write our hashset! and haspmap! instead of including an additional dependency, the implementation isn't that complicated, so we could write it by ourself.

You could take time to review it :)

[marioortizmanero]

Great! But I meant a specific macro for scopes, that even includes the .to_owned() part, and public for everyone to use. It could be a simple wrapper over your hashset macro, but we don't need a hashset macro for anything else so it could be a single macro as well.

[ramsayleung]

Yep, I have just replaced hashset! with Rspotify-scope specific macro scope! in this commit: f78abf5 (#189)
And usage of this macro will look like:

let scope: HashSet<String> = scope!("playlist-read-private", "playlist-read-collaborative");
let tok = TokenBuilder::default()
    .access_token("test-access_token")
    .expires_in(Duration::seconds(1))
    .expires_at(Utc::now())
    .scope(scope)
    .refresh_token("...")
    .build()
    .unwrap();

[marioortizmanero]

Can you use the new scope macro in the examples and other parts of code as well? Just search .scope( in the codebase

[marioortizmanero]

We can continue with this PR after merging the ID's PR. As to my review, the only missing things are:

• Could you mention the existence of the scope! macro in the token scope member? Those who don't take a look at the examples won't know about it. Here https://github.com/ramsayleung/rspotify/blob/master/src/oauth2.rs#L88, something like "you can use the macro scopes! to build it at compile time easily".

  Also now that I see it, #[builder(default = "HashSet::new()")] isn't needed, you can just change it to #[builder(default)]

• Lines 30-31 in macros.rs should be merged in the same one with the same indentation to keep it consistent (for some reason cargo fmt doesn't format macros automatically).

• The merge conflicts.

• Also, should the name be scope! or scopes!? I actually prefer the latter because it builds a set of scopes, not a single one. But not sure if you named it scope for a different reason

[ramsayleung]

Could you mention the existence of the scope! macro in the token scope member? Those who don't take a look at the examples won't know about it. Here https://github.com/ramsayleung/rspotify/blob/master/src/oauth2.rs#L88, something like "you can use the macro scopes! to build it at compile time easily".

Great idea.

Lines 30-31 in macros.rs should be merged in the same one with the same indentation to keep it consistent (for some reason cargo fmt doesn't format macros automatically)

I have the same problem that cargo fmt doesn't work for macros for unknown reason, so I have to format it manually

Also, should the name be scope! or scopes!? I actually prefer the latter because it builds a set of scopes, not a single one. But not sure if you named it scope for a different reason

Great catch. No special reason, I just forgot to take context into consideration, so simply came with this name scope!.

[marioortizmanero]

This PR can be merged after these suggestions, right?

[ramsayleung]

This PR can be merged after these suggestions, right?

Yep, this PR is ready to be merged :-)