rspotify icon indicating copy to clipboard operation
rspotify copied to clipboard

Added a builder like struct for the search function (Related with issue )

Open GartoxFR opened this issue 3 years ago • 8 comments

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).

GartoxFR avatar Oct 16 '22 15:10 GartoxFR

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

GartoxFR avatar Oct 19 '22 09:10 GartoxFR

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.

ramsayleung avatar Oct 24 '22 13:10 ramsayleung

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

GartoxFR avatar Nov 03 '22 13:11 GartoxFR

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

  1. Just making the order of the keys determined and predictable by replacing HashMap with BTreeMap which is a sorted map by itself. Then the complexity of querying if a key is in the map will reduce from O(1) to O(LogN), but I think it's acceptable.
  2. 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'));
// ...

ramsayleung avatar Nov 04 '22 12:11 ramsayleung

I finally chose the BTreeMap because it's a bit tricky to split correctly the query as there can be spaces inside album, track..

GartoxFR avatar Nov 08 '22 17:11 GartoxFR

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 ?

GartoxFR avatar Jan 18 '23 09:01 GartoxFR

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"
/// ```

ramsayleung avatar May 26 '23 15:05 ramsayleung

Message to comment on stale PRs. If none provided, will not mark PRs stale

github-actions[bot] avatar Nov 23 '23 02:11 github-actions[bot]