Remove Rspotify default parameters and add parameter macros

[marioortizmanero]

Description

• Leaves default parameter handling to Spotify, removing .unwrap_or
• Implements a macro for both query and json parameters so that endpoints are much nicer to read.
• Closes Optional parameters #134, we're consistently using Option<T> instead of Into<Opton<T>> now. If you don't like this please let me know, but I didn't get any more feedback so I went for the simpler option.

Motivation and Context

• Default values should be assigned by the Spotify API instead by us, clearly. We shouldn't touch things we don't have to.
• As Meta-Issue #127 said, endpoints should be as concise as possible because they're the most important part of the library after all, and it's important that they are easy to maintain.

Dependencies

None

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Please also list any relevant details for your test configuration

• There are a couple new tests for the macros

[ramsayleung]

Great job! I totally agree to your point, the default parameter in this crate is a little messy. Good to see the improvement.

Since the parameters in endpoints are mostly passed by value, then we are working on the function signature, shall we update the parameters to pass them by reference?

[marioortizmanero]

Since the parameters in endpoints are mostly passed by value, then we are working on the function signature, shall we update the parameters to pass them by reference?

I don't really know. For now I've only set those parameters that I needed by reference, and left the rest by value. I guess it would make sense, yeah.

This PR is now ready for review when you have time, by the way. It ended up much bigger than I thought it would haha. Let me know if you have any doubts/suggestions regarding the macros.

[ramsayleung]

For now I've only set those parameters that I needed by reference, and left the rest by value.

I think it will make the endpoint inconsistency, some parameters passed by reference, and the others by value. I am not sure if it's a chance to modify them all.

This PR is now ready for review when you have time, by the way.

Yes, I will.

[marioortizmanero]

I think it will make the endpoint inconsistency, some parameters passed by reference, and the others by value. I am not sure if it's a chance to modify them all.

I'd say go for it, it makes sense to me.

[marioortizmanero]

Something that's inconsistent about the build_map macro that I'm unsure about is that for optional parameters it takes a reference, and for required it's the value itself. This is because for optional parameters it may be expanded to:

if let Some(ref name) = name {
     params.insert("name", name);
}

But if this were a required parameter it would be:

params.insert("name", name);

The effect of this is that you can pass a String to an optional parameter and it will be converted to &str automatically, but this won't work for required parameters. This is required because we're using a HashMap<&str, &str> now, so sometimes if let Some(name) = name wouldn't work at all.

A way to make this consistent would be that required parameters were also taken as a reference:

params.insert("name", &name);

But this won't work for build_json because it expects a Value, not a &Value. So the only way to "fix" this inconsistency is to implement the macros so that build_json takes values, and build_map takes references.

[marioortizmanero]

Or another idea for the macro syntax, which solves the previous issue. What if it worked like this instead, imitating pattern matching?

let params = build_map! {
    name => name, // this is required
    Some(ref limit) => limit, // this is optional, and taken as a reference
    Some(limit) => limit, // this is optional, and taken as value
    Some(ref market) => market.as_ref() // you can still add a specific value
};

We don't need the required/optional keywords, at the cost at perhaps being a bit more "weird".

---

And the last option is:

let params = build_map! {
    required "name": name, // will always be inserted
    "name": name, // we could remove the `required` keyword
    optional "limit": limit,
    optional "market": market.map(|m| m.as_ref()), // this is like the  `=>` expressions
};

Which would expand to:

let mut map = HashMap::with_capacity(3);
map.insert("name", name);
if let Some(val) = limit {
    map.insert("limit", val);
}
if let Some(val) = market.map(|m| m.as_ref()) {
    map.insert("market", val);
}

This is the most intuitive because it uses literals as the keys. The downside is that it requires more code for optional members.

[marioortizmanero]

I think that the best macro syntax is the last one I mentioned, because I dislike magic macros that make you look their documentation in order to understand them. And in this case it's pretty much self-descriptive because it doesn't really hide anything, unlike the other two macros.

What do you think? Any other macro you can come up with? Maybe I'm overthinking it?

Sorry for thinking about other syntaxes after imlementing the macro, I didn't really notice the downsides of the current one until it was fully implemented.

[ramsayleung]

What do you think? Any other macro you can come up with?

I personally prefer the last one with same reason as you do. The other macro syntaxes are a little subtle and hide something into a black box. If you want to know about how it works, you have dig deep into it.

let params = build_map! {
    required "name": name, // will always be inserted
    "name": name, // we could remove the `required` keyword
    optional "limit": limit,
    optional "market": market.map(|m| m.as_ref()), // this is like the  `=>` expressions
};

And I think we could remove the required keyword, since we have explicitly used the optional keyword, on the opposite, the required keyword is not that necessary.

[marioortizmanero]

So, do you think the endpoints look better like this? In that case I think this is ready for a merge.

[ramsayleung]

Merged

So, do you think the endpoints look better like this? In that case I think this is ready for a merge.

Yep, both macros and endpoints look better than before :)