rspotify
rspotify copied to clipboard
Added a builder like struct for the search function (Related with issue )
Description
These changes add a struct to build well formatted query to pass to the search method (fixes #354). These are not breaking changes as you can still pass a simple String to the method. It can still be improve as it doesn't prevent users to pass wrong filter with wrong SearchType (give a track title to search an album for example, which is not supporte by Spotify API).
Motivation and Context
Make writing searches easier
Dependencies
Adding strum to the main project
Type of change
Please delete options that are not relevant.
- [x] New feature (non-breaking change which adds functionality)
- [X] This change requires a documentation update
How has this been tested?
I've modified one test case to use the struct instead of a basic &str. As the old test still passes, these changes are non breaking.
I also wrote a simple example which printed the String generated by the struct to verify that it was right with its inputs. This file is included in this PR but will probably be removed if it is accepted.
Please also list any relevant details for your test configuration
Is this change properly documented?
Please make sure you've properly documented the changes you're making.
Don't forget to add an entry to the CHANGELOG if necessary (new features, breaking changes, relevant internal improvements).
I tried an other approach to ensure type safety. Please tell me what you think about this before I try to implement testing and more example around it
After rereading the Spotify's document, I think its explanation about the restriction between filter and result type is ambiguous, so I think it's better to leave filter and result type alone, roll back to your original design.
Just adding a builder for the query string, and we are unnecessary to include the search type into the builder.
I reverted my previous changes to go back to the builder pattern. I'm not used to writing test so can you give me ideas on how to write test for this ? I can't just call the method into and compare the result to an expected query string because the order of the keys in the query string is undefined and can change
In my opinion, there are two ways to test the SearchQuery builder.
I can't just call the method into and compare the result to an expected query string because the order of the keys in the query string is undefined and can change
- Just making the order of the keys determined and predictable by replacing
HashMapwithBTreeMapwhich is a sorted map by itself. Then the complexity of querying if a key is in the map will reduce fromO(1)toO(LogN), but I think it's acceptable. - Test the query string word by word, for example:
let query_string = SearchQuery::default().album("arrival").artist("abba").tag_new("test");
// the query_string could be:
// 1. `ablum:arrival artist:abba test`
// 2. `artist:abba ablum:arrival test`
// 3. `test artist:abba ablum:arrival`
// 4. ...
// we could split query string by whitespace and assert the splited strings contain `ablum:arrival` `artist:abba` `test`
let query_words: Vec<String> = query_string.split(' ').collect();
assert_eq!(3, query_words.len());
assert!(query_words.contains('ablum:arrival'));
assert!(query_words.contains('artist:abba'));
// ...
I finally chose the BTreeMap because it's a bit tricky to split correctly the query as there can be spaces inside album, track..
I'm a bit confused about what did just happen in the pipeline. It seems it complained about some code which is inside the documentation. I never really worked with rust doc so what happened ?
The pipeline complained about that it's unable to find the definition of SearchQuery, so you should import the SearchQuery before using it:
/// Exemple
/// ```rust
/// use SearchQuery;
/// SearchQuery::default()
/// .any("foo")
/// .album("bar")
/// // Filter on album containing "bar" and anything containing "foo"
/// ```
Message to comment on stale PRs. If none provided, will not mark PRs stale