From 5e9086422c9a2d6d4a822f2daf9056511a374670 Mon Sep 17 00:00:00 2001 From: Meris Bahtijaragic Date: Wed, 9 Mar 2022 21:37:20 +0100 Subject: [PATCH 01/28] fix: start refactor --- rspotify-model/src/idtypes.rs | 39 ++++++++++++++++++----------------- rspotify-model/src/lib.rs | 10 ++++++--- rspotify-model/src/track.rs | 4 ++-- src/clients/oauth.rs | 18 ++++++++-------- 4 files changed, 38 insertions(+), 33 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 62f81e7f..de203a3b 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -270,9 +270,6 @@ pub trait Id: Send + Sync { } } -pub trait PlayableId: Id {} -pub trait PlayContextId: Id {} - /// This macro helps consistently define ID types. /// /// * The `$type` parameter indicates what type the ID is made out of (say, @@ -474,12 +471,16 @@ impl Id for UserId { define_impls!(ArtistId, AlbumId, TrackId, PlaylistId, UserId, ShowId, EpisodeId); // Grouping up the IDs into more specific traits -impl PlayContextId for ArtistId {} -impl PlayContextId for AlbumId {} -impl PlayContextId for PlaylistId {} -impl PlayContextId for ShowId {} -impl PlayableId for TrackId {} -impl PlayableId for EpisodeId {} +pub enum PlayContext { + Artist(ArtistId), + Album(AlbumId), + Playlist(PlaylistId), + Show(ShowId), +} +pub enum Playable { + Track(TrackId), + Episode(EpisodeId), +} #[cfg(test)] mod test { @@ -565,27 +566,27 @@ mod test { #[test] fn test_multiple_types() { - fn endpoint(_ids: impl IntoIterator>) {} + fn endpoint(_ids: impl IntoIterator) {} - let tracks: [Box; 4] = [ - Box::new(TrackId::from_id(ID).unwrap()), - Box::new(TrackId::from_id(ID).unwrap()), - Box::new(EpisodeId::from_id(ID).unwrap()), - Box::new(EpisodeId::from_id(ID).unwrap()), + let tracks: Vec = vec![ + Playable::Track(TrackId::from_id(ID).unwrap()), + Playable::Track(TrackId::from_id(ID).unwrap()), + Playable::Episode(EpisodeId::from_id(ID).unwrap()), + Playable::Episode(EpisodeId::from_id(ID).unwrap()), ]; endpoint(tracks); } #[test] fn test_unknown_at_compile_time() { - fn endpoint1(input: &str, is_episode: bool) -> Box { + fn endpoint1(input: &str, is_episode: bool) -> Playable { if is_episode { - Box::new(EpisodeId::from_id(input).unwrap()) + Playable::Episode(EpisodeId::from_id(input).unwrap()) } else { - Box::new(TrackId::from_id(input).unwrap()) + Playable::Track(TrackId::from_id(input).unwrap()) } } - fn endpoint2(_id: &[Box]) {} + fn endpoint2(_id: &[Playable]) {} let id = endpoint1(ID, false); endpoint2(&[id]); diff --git a/rspotify-model/src/lib.rs b/rspotify-model/src/lib.rs index f4012837..1fd49fc5 100644 --- a/rspotify-model/src/lib.rs +++ b/rspotify-model/src/lib.rs @@ -53,10 +53,14 @@ impl PlayableItem { /// /// Note that if it's a track and if it's local, it may not have an ID, in /// which case this function will return `None`. - pub fn id(&self) -> Option<&dyn PlayableId> { + pub fn id(&self) -> Option { match self { - PlayableItem::Track(t) => t.id.as_ref().map(|t| t as &dyn PlayableId), - PlayableItem::Episode(e) => Some(&e.id), + PlayableItem::Track(t) => { + t.id.clone() + .and_then(|x| TrackId::from_id(x.id()).ok()) + .map(Playable::Track) + } + PlayableItem::Episode(e) => EpisodeId::from_id(e.id.id()).ok().map(Playable::Episode), } } } diff --git a/rspotify-model/src/track.rs b/rspotify-model/src/track.rs index f684ad68..bf924e04 100644 --- a/rspotify-model/src/track.rs +++ b/rspotify-model/src/track.rs @@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize}; use std::{collections::HashMap, time::Duration}; use crate::{ - custom_serde::duration_ms, PlayableId, Restriction, SimplifiedAlbum, SimplifiedArtist, TrackId, + custom_serde::duration_ms, Playable, Restriction, SimplifiedAlbum, SimplifiedArtist, TrackId, }; /// Full track object @@ -90,6 +90,6 @@ pub struct SavedTrack { /// PlayableId` instead of `Box` to avoid the unnecessary /// allocation. Same goes for the positions slice instead of vector. pub struct ItemPositions<'a> { - pub id: &'a dyn PlayableId, + pub id: Playable, pub positions: &'a [u32], } diff --git a/src/clients/oauth.rs b/src/clients/oauth.rs index 61d3c307..35cfcb9d 100644 --- a/src/clients/oauth.rs +++ b/src/clients/oauth.rs @@ -14,7 +14,7 @@ use crate::{ use std::{collections::HashMap, time}; use maybe_async::maybe_async; -use rspotify_model::idtypes::PlayContextId; +use rspotify_model::idtypes::{PlayContext, Playable}; use serde_json::{json, Map}; use url::Url; @@ -296,7 +296,7 @@ pub trait OAuthClient: BaseClient { async fn playlist_add_items<'a>( &self, playlist_id: &PlaylistId, - items: impl IntoIterator + Send + 'a, + items: impl IntoIterator + Send + 'a, position: Option, ) -> ClientResult { let uris = items.into_iter().map(|id| id.uri()).collect::>(); @@ -321,7 +321,7 @@ pub trait OAuthClient: BaseClient { async fn playlist_replace_items<'a>( &self, playlist_id: &PlaylistId, - items: impl IntoIterator + Send + 'a, + items: impl IntoIterator + Send + 'a, ) -> ClientResult<()> { let uris = items.into_iter().map(|id| id.uri()).collect::>(); let params = build_json! { @@ -377,7 +377,7 @@ pub trait OAuthClient: BaseClient { async fn playlist_remove_all_occurrences_of_items<'a>( &self, playlist_id: &PlaylistId, - track_ids: impl IntoIterator + Send + 'a, + track_ids: impl IntoIterator + Send + 'a, snapshot_id: Option<&str>, ) -> ClientResult { let tracks = track_ids @@ -1025,9 +1025,9 @@ pub trait OAuthClient: BaseClient { /// - position_ms - Indicates from what position to start playback. /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/start-a-users-playback) - async fn start_context_playback( + async fn start_context_playback( &self, - context_uri: &T, + context_uri: PlayContext, device_id: Option<&str>, offset: Option, position_ms: Option, @@ -1059,7 +1059,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/start-a-users-playback) async fn start_uris_playback<'a>( &self, - uris: impl IntoIterator + Send + 'a, + uris: impl IntoIterator + Send + 'a, device_id: Option<&str>, offset: Option, position_ms: Option, @@ -1218,9 +1218,9 @@ pub trait OAuthClient: BaseClient { /// targeted /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/add-to-queue) - async fn add_item_to_queue( + async fn add_item_to_queue( &self, - item: &T, + item: &Playable, device_id: Option<&str>, ) -> ClientResult<()> { let url = append_device_id(&format!("me/player/queue?uri={}", item.uri()), device_id); From a6c0654b996482cfac6fad239756aa6aded6fbc3 Mon Sep 17 00:00:00 2001 From: Meris Bahtijaragic Date: Wed, 9 Mar 2022 22:00:19 +0100 Subject: [PATCH 02/28] fix some more refactor --- rspotify-model/src/idtypes.rs | 20 ++++++++++++++++++++ src/lib.rs | 2 +- tests/test_with_oauth.rs | 32 ++++++++++++++++---------------- 3 files changed, 37 insertions(+), 17 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index de203a3b..0b795199 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -482,6 +482,26 @@ pub enum Playable { Episode(EpisodeId), } +impl Playable { + pub fn uri(&self) -> String { + match self { + Playable::Track(t) => t.uri(), + Playable::Episode(e) => e.uri(), + } + } +} + +impl PlayContext { + pub fn uri(&self) -> String { + match self { + PlayContext::Album(x) => x.uri(), + PlayContext::Artist(x) => x.uri(), + PlayContext::Playlist(x) => x.uri(), + PlayContext::Show(x) => x.uri(), + } + } +} + #[cfg(test)] mod test { use super::*; diff --git a/src/lib.rs b/src/lib.rs index 0da9dc15..cbb9fc2e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -151,7 +151,7 @@ use thiserror::Error; pub mod prelude { pub use crate::clients::{BaseClient, OAuthClient}; - pub use crate::model::idtypes::{Id, PlayContextId, PlayableId}; + pub use crate::model::idtypes::{Id, PlayContext, Playable}; } /// Common headers as constants. diff --git a/tests/test_with_oauth.rs b/tests/test_with_oauth.rs index db6b4038..e3d82003 100644 --- a/tests/test_with_oauth.rs +++ b/tests/test_with_oauth.rs @@ -638,11 +638,11 @@ async fn check_num_tracks(client: &AuthCodeSpotify, playlist_id: &PlaylistId, nu #[maybe_async] async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist) { // The tracks in the playlist, some of them repeated - let tracks: [&dyn PlayableId; 4] = [ - &TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap(), - &TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap(), - &EpisodeId::from_uri("spotify/episode/381XrGKkcdNkLwfsQ4Mh5y").unwrap(), - &EpisodeId::from_uri("spotify/episode/6O63eWrfWPvN41CsSyDXve").unwrap(), + let tracks = vec![ + Playable::Track(TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap()), + Playable::Track(TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap()), + Playable::Episode(EpisodeId::from_uri("spotify/episode/381XrGKkcdNkLwfsQ4Mh5y").unwrap()), + Playable::Episode(EpisodeId::from_uri("spotify/episode/6O63eWrfWPvN41CsSyDXve").unwrap()), ]; // Firstly adding some tracks @@ -661,14 +661,14 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist check_num_tracks(client, &playlist.id, tracks.len() as i32).await; // Replacing the tracks - let replaced_tracks: [&dyn PlayableId; 7] = [ - &TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap(), - &TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap(), - &TrackId::from_uri("spotify:track:1301WleyT98MSxVHPZCA6M").unwrap(), - &EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap(), - &TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap(), - &EpisodeId::from_id("4zugY5eJisugQj9rj8TYuh").unwrap(), - &TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap(), + let replaced_tracks = vec![ + Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), + Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), + Playable::Track(TrackId::from_uri("spotify:track:1301WleyT98MSxVHPZCA6M").unwrap()), + Playable::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), + Playable::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), + Playable::Episode(EpisodeId::from_id("4zugY5eJisugQj9rj8TYuh").unwrap()), + Playable::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), ]; client .playlist_replace_items(&playlist.id, replaced_tracks) @@ -696,9 +696,9 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist check_num_tracks(client, &playlist.id, replaced_tracks.len() as i32 - 3).await; // Removes all occurrences of two tracks - let to_remove: [&dyn PlayableId; 2] = [ - &TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap(), - &EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap(), + let to_remove: Vec![PlayableId] = [ + Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), + Playable::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), ]; client .playlist_remove_all_occurrences_of_items(&playlist.id, to_remove, None) From 62ba63f61c4abf50381887677aff96c6bc43435a Mon Sep 17 00:00:00 2001 From: Meris Bahtijaragic Date: Thu, 10 Mar 2022 09:10:04 +0100 Subject: [PATCH 03/28] fix: it compiles --- rspotify-model/src/idtypes.rs | 2 ++ tests/test_with_oauth.rs | 32 +++++++++++++++++++------------- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 0b795199..3bd007d4 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -471,12 +471,14 @@ impl Id for UserId { define_impls!(ArtistId, AlbumId, TrackId, PlaylistId, UserId, ShowId, EpisodeId); // Grouping up the IDs into more specific traits +#[derive(Clone)] pub enum PlayContext { Artist(ArtistId), Album(AlbumId), Playlist(PlaylistId), Show(ShowId), } +#[derive(Clone)] pub enum Playable { Track(TrackId), Episode(EpisodeId), diff --git a/tests/test_with_oauth.rs b/tests/test_with_oauth.rs index e3d82003..08e2e78d 100644 --- a/tests/test_with_oauth.rs +++ b/tests/test_with_oauth.rs @@ -323,10 +323,10 @@ async fn test_new_releases_with_from_token() { #[ignore] async fn test_playback() { let client = oauth_client().await; - let uris: [&dyn PlayableId; 3] = [ - &TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap(), - &TrackId::from_uri("spotify:track:2DzSjFQKetFhkFCuDWhioi").unwrap(), - &EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap(), + let uris = [ + Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), + Playable::Track(TrackId::from_uri("spotify:track:2DzSjFQKetFhkFCuDWhioi").unwrap()), + Playable::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), ]; let devices = client.device().await.unwrap(); @@ -346,7 +346,12 @@ async fn test_playback() { // Starting playback of some songs client - .start_uris_playback(uris, Some(device_id), Some(Offset::for_position(0)), None) + .start_uris_playback( + uris.to_vec(), + Some(device_id), + Some(Offset::for_position(0)), + None, + ) .await .unwrap(); @@ -638,7 +643,7 @@ async fn check_num_tracks(client: &AuthCodeSpotify, playlist_id: &PlaylistId, nu #[maybe_async] async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist) { // The tracks in the playlist, some of them repeated - let tracks = vec![ + let tracks = [ Playable::Track(TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap()), Playable::Track(TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap()), Playable::Episode(EpisodeId::from_uri("spotify/episode/381XrGKkcdNkLwfsQ4Mh5y").unwrap()), @@ -647,7 +652,7 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist // Firstly adding some tracks client - .playlist_add_items(&playlist.id, tracks, None) + .playlist_add_items(&playlist.id, tracks.to_vec(), None) .await .unwrap(); check_num_tracks(client, &playlist.id, tracks.len() as i32).await; @@ -661,7 +666,7 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist check_num_tracks(client, &playlist.id, tracks.len() as i32).await; // Replacing the tracks - let replaced_tracks = vec![ + let replaced_tracks = [ Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), Playable::Track(TrackId::from_uri("spotify:track:1301WleyT98MSxVHPZCA6M").unwrap()), @@ -671,7 +676,7 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist Playable::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), ]; client - .playlist_replace_items(&playlist.id, replaced_tracks) + .playlist_replace_items(&playlist.id, replaced_tracks.to_vec()) .await .unwrap(); // Making sure the number of tracks is updated @@ -680,11 +685,11 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist // Removes a few specific tracks let tracks = [ ItemPositions { - id: &TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap(), + id: Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), positions: &[0], }, ItemPositions { - id: &TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap(), + id: Playable::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), positions: &[4, 6], }, ]; @@ -696,7 +701,7 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist check_num_tracks(client, &playlist.id, replaced_tracks.len() as i32 - 3).await; // Removes all occurrences of two tracks - let to_remove: Vec![PlayableId] = [ + let to_remove = vec![ Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), Playable::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), ]; @@ -763,7 +768,8 @@ async fn test_volume() { async fn test_add_queue() { // NOTE: unfortunately it's impossible to revert this test - let birdy_uri = TrackId::from_uri("spotify:track:6rqhFgbbKwnb9MLmUQDhG6").unwrap(); + let birdy_uri = + Playable::Track(TrackId::from_uri("spotify:track:6rqhFgbbKwnb9MLmUQDhG6").unwrap()); oauth_client() .await .add_item_to_queue(&birdy_uri, None) From e43c253c7cda2ef376e7fae2f16af8989b63bf23 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Wed, 25 May 2022 23:41:22 +0200 Subject: [PATCH 04/28] More performant version done! --- rspotify-model/Cargo.toml | 2 +- rspotify-model/src/album.rs | 8 +- rspotify-model/src/artist.rs | 6 +- rspotify-model/src/audio.rs | 4 +- rspotify-model/src/idtypes.rs | 363 +++++++++++++++++++-------------- rspotify-model/src/lib.rs | 10 +- rspotify-model/src/playlist.rs | 6 +- rspotify-model/src/show.rs | 10 +- rspotify-model/src/track.rs | 17 +- rspotify-model/src/user.rs | 6 +- src/clients/oauth.rs | 14 +- src/lib.rs | 2 +- 12 files changed, 255 insertions(+), 193 deletions(-) diff --git a/rspotify-model/Cargo.toml b/rspotify-model/Cargo.toml index fa60194d..7af6ef27 100644 --- a/rspotify-model/Cargo.toml +++ b/rspotify-model/Cargo.toml @@ -15,8 +15,8 @@ readme = "../README.md" [dependencies] chrono = { version = "0.4.19", features = ["serde", "rustc-serialize"] } +enum_dispatch = "0.3.8" serde = { version = "1.0.130", features = ["derive"] } serde_json = "1.0.67" strum = { version = "0.24.0", features = ["derive"] } thiserror = "1.0.29" - diff --git a/rspotify-model/src/album.rs b/rspotify-model/src/album.rs index 374ef819..d4023ae8 100644 --- a/rspotify-model/src/album.rs +++ b/rspotify-model/src/album.rs @@ -6,8 +6,8 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use crate::{ - AlbumId, AlbumType, Copyright, DatePrecision, Image, Page, RestrictionReason, SimplifiedArtist, - SimplifiedTrack, + AlbumIdBuf, AlbumType, Copyright, DatePrecision, Image, Page, RestrictionReason, + SimplifiedArtist, SimplifiedTrack, }; /// Simplified Album Object @@ -21,7 +21,7 @@ pub struct SimplifiedAlbum { pub available_markets: Vec, pub external_urls: HashMap, pub href: Option, - pub id: Option, + pub id: Option, pub images: Vec, pub name: String, #[serde(skip_serializing_if = "Option::is_none")] @@ -43,7 +43,7 @@ pub struct FullAlbum { pub external_urls: HashMap, pub genres: Vec, pub href: String, - pub id: AlbumId, + pub id: AlbumIdBuf, pub images: Vec, pub name: String, pub popularity: u32, diff --git a/rspotify-model/src/artist.rs b/rspotify-model/src/artist.rs index 42d11e4b..a45ee346 100644 --- a/rspotify-model/src/artist.rs +++ b/rspotify-model/src/artist.rs @@ -4,14 +4,14 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; -use crate::{ArtistId, CursorBasedPage, Followers, Image}; +use crate::{ArtistIdBuf, CursorBasedPage, Followers, Image}; /// Simplified Artist Object #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Default)] pub struct SimplifiedArtist { pub external_urls: HashMap, pub href: Option, - pub id: Option, + pub id: Option, pub name: String, } @@ -22,7 +22,7 @@ pub struct FullArtist { pub followers: Followers, pub genres: Vec, pub href: String, - pub id: ArtistId, + pub id: ArtistIdBuf, pub images: Vec, pub name: String, pub popularity: u32, diff --git a/rspotify-model/src/audio.rs b/rspotify-model/src/audio.rs index 169fc54e..305a971c 100644 --- a/rspotify-model/src/audio.rs +++ b/rspotify-model/src/audio.rs @@ -6,7 +6,7 @@ use std::time::Duration; use crate::{ custom_serde::{duration_ms, modality}, - Modality, TrackId, + Modality, TrackIdBuf, }; /// Audio Feature Object @@ -18,7 +18,7 @@ pub struct AudioFeatures { #[serde(with = "duration_ms", rename = "duration_ms")] pub duration: Duration, pub energy: f32, - pub id: TrackId, + pub id: TrackIdBuf, pub instrumentalness: f32, pub key: i32, pub liveness: f32, diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 3bd007d4..971a86a2 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -128,18 +128,16 @@ pub enum IdError { /// Unfortunately, since `where Self: Sized` has to be enforced, IDs cannot be /// simply a `str` internally because these aren't sized. Thus, IDs will have the /// slight overhead of having to use an owned type like `String`. -pub trait Id: Send + Sync { +pub trait Id { + /// The type of the ID. + const TYPE: Type; + /// Spotify object Id (guaranteed to be valid for that type) fn id(&self) -> &str; - /// The type of the ID. The difference with [`Self::_type_static`] is that - /// this method can be used so that `Id` is an object-safe trait. - fn _type(&self) -> Type; - - /// The type of the ID, which can be used without initializing it - fn _type_static() -> Type - where - Self: Sized; + /// Only returns `true` in case the given string is valid according to that + /// specific Id (e.g., some may require alphanumeric characters only). + fn id_is_valid(id: &str) -> bool; /// Initialize the Id without checking its validity. /// @@ -147,16 +145,14 @@ pub trait Id: Send + Sync { /// /// The string passed to this method must be made out of valid characters /// only; otherwise undefined behaviour may occur. - unsafe fn from_id_unchecked(id: &str) -> Self - where - Self: Sized; + unsafe fn from_id_unchecked(id: &str) -> &Self; /// Spotify object URI in a well-known format: `spotify:type:id` /// /// Examples: `spotify:album:6IcGNaXFRf5Y1jc7QsE9O2`, /// `spotify:track:4y4VO05kYgUTo2bzbox1an`. fn uri(&self) -> String { - format!("spotify:{}:{}", self._type(), self.id()) + format!("spotify:{}:{}", Self::TYPE, self.id()) } /// Full Spotify object URL, can be opened in a browser @@ -164,10 +160,10 @@ pub trait Id: Send + Sync { /// Examples: `https://open.spotify.com/track/4y4VO05kYgUTo2bzbox1an`, /// `https://open.spotify.com/artist/2QI8e2Vwgg9KXOz2zjcrkI`. fn url(&self) -> String { - format!("https://open.spotify.com/{}/{}", self._type(), self.id()) + format!("https://open.spotify.com/{}/{}", Self::TYPE, self.id()) } - /// Parse Spotify id from string slice + /// Parse Spotify Id from string slice. /// /// A valid Spotify object id must be a non-empty string with valid /// characters. @@ -175,10 +171,7 @@ pub trait Id: Send + Sync { /// # Errors: /// /// - `IdError::InvalidId` - if `id` contains invalid characters. - fn from_id(id: &str) -> Result - where - Self: Sized, - { + fn from_id(id: &str) -> Result<&Self, IdError> { if id.chars().all(|ch| ch.is_ascii_alphanumeric()) { // Safe, we've just checked that the Id is valid. Ok(unsafe { Self::from_id_unchecked(id) }) @@ -206,10 +199,7 @@ pub trait Id: Send + Sync { /// - `IdError::InvalidId` - if id part of an `uri` is not a valid id, /// - `IdError::InvalidFormat` - if it can't be splitted into type and /// id parts. - fn from_uri(uri: &str) -> Result - where - Self: Sized, - { + fn from_uri(uri: &str) -> Result<&Self, IdError> { let mut chars = uri .strip_prefix("spotify") .ok_or(IdError::InvalidPrefix)? @@ -228,7 +218,7 @@ pub trait Id: Send + Sync { // Note that in case the type isn't known at compile time, any type will // be accepted. match tpe.parse::() { - Ok(tpe) if tpe == Self::_type_static() => Self::from_id(&id[1..]), + Ok(tpe) if tpe == Self::TYPE => Self::from_id(&id[1..]), _ => Err(IdError::InvalidType), } } @@ -258,10 +248,7 @@ pub trait Id: Send + Sync { /// characters), /// - `IdError::InvalidFormat` - if `id_or_uri` is an URI, and it can't be /// split into type and id parts. - fn from_id_or_uri(id_or_uri: &str) -> Result - where - Self: Sized, - { + fn from_id_or_uri(id_or_uri: &str) -> Result<&Self, IdError> { match Self::from_uri(id_or_uri) { Ok(id) => Ok(id), Err(IdError::InvalidPrefix) => Self::from_id(id_or_uri), @@ -275,60 +262,76 @@ pub trait Id: Send + Sync { /// * The `$type` parameter indicates what type the ID is made out of (say, /// `Artist`, `Album`...) from the enum `Type`. /// * The `$name` parameter is the identifier of the struct for that type. +/// +/// This macro contains a lot of code but mostly it's just repetitive work to +/// implement some common traits that's not of much interest for being trivial. +/// +/// * The `$name` parameter is the identifier of the struct for that type. macro_rules! define_idtypes { - ($($type:ident => $name:ident),+) => { + ($($type:ident => { + borrowed: $borrowed:ident, + owned: $owned:ident, + validity: $validity:expr + }),+) => { $( #[doc = concat!( - "ID of type [`Type::", stringify!($type), "`], made up of only \ - alphanumeric characters. Refer to the [module-level \ - docs][`crate::idtypes`] for more information.") - ] - #[derive(Clone, Debug, PartialEq, Eq, Serialize, Hash)] - pub struct $name(String); - - impl Id for $name { + "Borrowed ID of type [`Type::", stringify!($type), "`]. Its \ + implementation of `id_is_valid` is defined by the closure `", + stringify!($validity), "`.\nRefer to the [module-level \ + docs][`crate::idtypes`] for more information. " + )] + #[repr(transparent)] + #[derive(Debug, PartialEq, Eq, Serialize, Hash)] + pub struct $borrowed(str); + + impl Id for $borrowed { + const TYPE: Type = Type::$type; + #[inline] fn id(&self) -> &str { &self.0 } #[inline] - fn _type(&self) -> Type { - Type::$type + fn id_is_valid(id: &str) -> bool { + // TODO: make prettier? + const VALID_FN: fn(&str) -> bool = $validity; + VALID_FN(id) } #[inline] - fn _type_static() -> Type where Self: Sized { - Type::$type - } - - #[inline] - unsafe fn from_id_unchecked(id: &str) -> Self { - $name(id.to_owned()) + unsafe fn from_id_unchecked(id: &str) -> &Self { + // SAFETY: this is okay because Ids are transparent over + // their inner type, a `str`. It would be equivalent to a + // `transmute`. See: + // https://doc.rust-lang.org/nomicon/casts.html + &*(id as *const str as *const Self) } } - )+ - } -} -/// This macro contains a lot of code but mostly it's just repetitive work to -/// implement some common traits that's not of much interest for being trivial. -/// -/// * The `$name` parameter is the identifier of the struct for that type. -macro_rules! define_impls { - ($($name:ident),+) => { - $( + #[doc = concat!( + "Owned ID of type [`Type::", stringify!($type), "`], made up \ + of only alphanumeric characters. Note that we don't really \ + need to change the contents of the URI string, so we can use a \ + `Box` instead of a `String` for its owned variant. Not \ + only does this make it easier to implement and use, but also \ + it reduces its size (on my machine it's 16 bytes vs 24 bytes). \ + Refer to the [module-level docs][`crate::idtypes`] for more \ + information." + )] + pub type $owned = Box<$borrowed>; + // Deserialization may take either an Id or an URI, so its // implementation has to be done manually. - impl<'de> Deserialize<'de> for $name { - fn deserialize(deserializer: D) -> Result<$name, D::Error> + impl<'de> Deserialize<'de> for $owned { + fn deserialize(deserializer: D) -> Result where D: serde::Deserializer<'de>, { struct IdVisitor; impl<'de> serde::de::Visitor<'de> for IdVisitor { - type Value = $name; + type Value = $owned; fn expecting( &self, formatter: &mut std::fmt::Formatter<'_> @@ -343,8 +346,9 @@ macro_rules! define_impls { where E: serde::de::Error, { - $name::from_id_or_uri(value) - .map_err(serde::de::Error::custom) + $borrowed::from_id_or_uri(value) + .map(ToOwned::to_owned) + .map_err(serde::de::Error::custom) } #[inline] @@ -368,7 +372,8 @@ macro_rules! define_impls { { let field = seq.next_element()? .ok_or_else(|| serde::de::Error::invalid_length(0, &self))?; - $name::from_id_or_uri(field) + $borrowed::from_id_or_uri(field) + .map(ToOwned::to_owned) .map_err(serde::de::Error::custom) } } @@ -378,7 +383,7 @@ macro_rules! define_impls { } /// Cheap conversion to `str` - impl AsRef for $name { + impl AsRef for $borrowed { fn as_ref(&self) -> &str { self.id() } @@ -386,26 +391,53 @@ macro_rules! define_impls { /// `Id`s may be borrowed as `str` the same way `Box` may be /// borrowed as `T` or `String` as `str` - impl std::borrow::Borrow for $name { + impl std::borrow::Borrow for $borrowed { fn borrow(&self) -> &str { self.id() } } /// Displaying the ID shows its URI - impl std::fmt::Display for $name { + impl std::fmt::Display for $borrowed { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { write!(f, "{}", self.uri()) } } - /// IDs can also be used to convert from a `str`; this works both - /// with IDs and URIs. - impl std::str::FromStr for $name { - type Err = IdError; + impl std::borrow::ToOwned for $borrowed { + type Owned = $owned; + + fn to_owned(&self) -> Self::Owned { + todo!() + } + } + + /// `Box` implements clone, but that doesn't cover `Box` + /// where `T` is a newtype for `str`. Thus, this manually delegates + /// `Clone` for `Box` to the implementation for `Box`. + impl Clone for $owned { + fn clone(&self) -> $owned { + // SAFETY: this is okay because Ids are transparent over + // their inner type, a `str`. See: + // https://doc.rust-lang.org/stable/nomicon/transmutes.html + let id: &Box = unsafe { std::mem::transmute(self) }; + + // Calling `Box::clone`, which is implemented + let id = id.clone(); + + // SAFETY: same as above, but in the inverse direction. + unsafe { std::mem::transmute(id) } + } + } + + /// Exactly the same thing in `Clone` happens with `Default`. + impl Default for $owned { + fn default() -> $owned { + // Calling `Box::default`, which is implemented + let id: Box = Default::default(); - fn from_str(s: &str) -> Result { - Self::from_id_or_uri(s) + // SAFETY: same as in `Clone`. + unsafe { std::mem::transmute(id) } } } )+ @@ -415,94 +447,127 @@ macro_rules! define_impls { // First declaring the regular IDs. Those with custom behaviour will have to be // declared manually later on. define_idtypes!( - Artist => ArtistId, - Album => AlbumId, - Track => TrackId, - Playlist => PlaylistId, - Show => ShowId, - Episode => EpisodeId + Artist => { + borrowed: ArtistId, + owned: ArtistIdBuf, + validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) + }, + Album => { + borrowed: AlbumId, + owned: AlbumIdBuf, + validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) + }, + Track => { + borrowed: TrackId, + owned: TrackIdBuf, + validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) + }, + Playlist => { + borrowed: PlaylistId, + owned: PlaylistIdBuf, + validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) + }, + Show => { + borrowed: ShowId, + owned: ShowIdBuf, + validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) + }, + Episode => { + borrowed: EpisodeId, + owned: EpisodeIdBuf, + validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) + }, + User => { + borrowed: UserId, + owned: UserIdBuf, + validity: |_| true + } ); -/// ID of type [`Type::User`]. Refer to the [module-level -/// docs][`crate::idtypes`] for more information. -/// -/// Note that the implementation of this specific ID differs from the rest: a -/// user's ID doesn't necessarily have to be made of alphanumeric characters. It -/// can also use underscores and other characters, but since Spotify doesn't -/// specify it explicitly, this just allows any string as an ID. -#[derive(Clone, Debug, PartialEq, Eq, Serialize, Hash)] -pub struct UserId(String); -impl Id for UserId { - #[inline] - fn id(&self) -> &str { - &self.0 - } +macro_rules! define_idgroups { + ($(pub enum ($borrowed:ident, $owned:ident) { + $( + $variant_name:ident => ($variant_borrowed:ident, $variant_owned:ident) + ),+ + }),+) => { + $( + pub enum $borrowed<'a> { + $( + $variant_name(&'a $variant_borrowed), + )+ + } - #[inline] - fn _type(&self) -> Type { - Type::User - } + pub enum $owned { + $( + $variant_name($variant_owned), + )+ + } - #[inline] - fn _type_static() -> Type - where - Self: Sized, - { - Type::User - } - #[inline] - unsafe fn from_id_unchecked(id: &str) -> Self { - UserId(id.to_owned()) - } + impl<'a> $borrowed<'a> { + pub fn uri(&self) -> String { + match self { + $( + $borrowed::$variant_name(x) => x.uri(), + )+ + } + } + pub fn url(&self) -> String { + match self { + $( + $borrowed::$variant_name(x) => x.url(), + )+ + } + } + pub const fn _type(&self) -> Type { + match self { + $( + $borrowed::$variant_name(_) => $variant_borrowed::TYPE, + )+ + } + } + } - /// Parse Spotify id from string slice. Spotify doesn't specify what a User - /// ID might look like, so this will allow any kind of value. - fn from_id(id: &str) -> Result - where - Self: Sized, - { - // Safe, we've just checked that the Id is valid. - Ok(unsafe { Self::from_id_unchecked(id) }) + impl $owned { + pub fn uri(&self) -> String { + match self { + $( + $owned::$variant_name(x) => x.uri(), + )+ + } + } + pub fn url(&self) -> String { + match self { + $( + $owned::$variant_name(x) => x.url(), + )+ + } + } + pub const fn _type(&self) -> Type { + match self { + $( + $owned::$variant_name(_) => $variant_borrowed::TYPE, + )+ + } + } + } + )+ } } -// Finally implement some common traits for the ID types. -define_impls!(ArtistId, AlbumId, TrackId, PlaylistId, UserId, ShowId, EpisodeId); - // Grouping up the IDs into more specific traits -#[derive(Clone)] -pub enum PlayContext { - Artist(ArtistId), - Album(AlbumId), - Playlist(PlaylistId), - Show(ShowId), -} -#[derive(Clone)] -pub enum Playable { - Track(TrackId), - Episode(EpisodeId), -} - -impl Playable { - pub fn uri(&self) -> String { - match self { - Playable::Track(t) => t.uri(), - Playable::Episode(e) => e.uri(), - } - } -} - -impl PlayContext { - pub fn uri(&self) -> String { - match self { - PlayContext::Album(x) => x.uri(), - PlayContext::Artist(x) => x.uri(), - PlayContext::Playlist(x) => x.uri(), - PlayContext::Show(x) => x.uri(), - } +define_idgroups!( + pub enum (PlayContextId, PlayContextIdBuf) { + Artist => (ArtistId, ArtistIdBuf), + Album => (AlbumId, AlbumIdBuf), + Playlist => (PlaylistId, PlaylistIdBuf), + Show => (ShowId, ShowIdBuf) + }, + pub enum (PlayableId, PlayableIdBuf) { + Track => (TrackId, TrackIdBuf), + Episode => (EpisodeId, EpisodeIdBuf) } -} +); #[cfg(test)] mod test { @@ -549,7 +614,7 @@ mod test { fn test_id_or_uri_and_deserialize() { fn test_any(check: F) where - F: Fn(&str) -> Result, + F: Fn(&str) -> Result<&TrackId, E>, E: Error, { // In this case we also check that the contents are the ID and not diff --git a/rspotify-model/src/lib.rs b/rspotify-model/src/lib.rs index 1fd49fc5..49ada28a 100644 --- a/rspotify-model/src/lib.rs +++ b/rspotify-model/src/lib.rs @@ -53,14 +53,10 @@ impl PlayableItem { /// /// Note that if it's a track and if it's local, it may not have an ID, in /// which case this function will return `None`. - pub fn id(&self) -> Option { + pub fn id(&self) -> Option> { match self { - PlayableItem::Track(t) => { - t.id.clone() - .and_then(|x| TrackId::from_id(x.id()).ok()) - .map(Playable::Track) - } - PlayableItem::Episode(e) => EpisodeId::from_id(e.id.id()).ok().map(Playable::Episode), + PlayableItem::Track(t) => t.id.as_ref().map(|id| PlayableId::Track(id)), + PlayableItem::Episode(e) => Some(PlayableId::Episode(&e.id)), } } } diff --git a/rspotify-model/src/playlist.rs b/rspotify-model/src/playlist.rs index 00e51387..b220d8d8 100644 --- a/rspotify-model/src/playlist.rs +++ b/rspotify-model/src/playlist.rs @@ -5,7 +5,7 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; -use crate::{Followers, Image, Page, PlayableItem, PlaylistId, PublicUser}; +use crate::{Followers, Image, Page, PlayableItem, PlaylistIdBuf, PublicUser}; /// Playlist result object #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Default)] @@ -26,7 +26,7 @@ pub struct SimplifiedPlaylist { pub collaborative: bool, pub external_urls: HashMap, pub href: String, - pub id: PlaylistId, + pub id: PlaylistIdBuf, pub images: Vec, pub name: String, pub owner: PublicUser, @@ -43,7 +43,7 @@ pub struct FullPlaylist { pub external_urls: HashMap, pub followers: Followers, pub href: String, - pub id: PlaylistId, + pub id: PlaylistIdBuf, pub images: Vec, pub name: String, pub owner: PublicUser, diff --git a/rspotify-model/src/show.rs b/rspotify-model/src/show.rs index bcd05889..d64a2ca1 100644 --- a/rspotify-model/src/show.rs +++ b/rspotify-model/src/show.rs @@ -4,7 +4,7 @@ use std::collections::HashMap; use std::time::Duration; use crate::{ - custom_serde::duration_ms, CopyrightType, DatePrecision, EpisodeId, Image, Page, ShowId, + custom_serde::duration_ms, CopyrightType, DatePrecision, EpisodeIdBuf, Image, Page, ShowIdBuf, }; /// Copyright object @@ -24,7 +24,7 @@ pub struct SimplifiedShow { pub explicit: bool, pub external_urls: HashMap, pub href: String, - pub id: ShowId, + pub id: ShowIdBuf, pub images: Vec, pub is_externally_hosted: Option, pub languages: Vec, @@ -56,7 +56,7 @@ pub struct FullShow { pub episodes: Page, pub external_urls: HashMap, pub href: String, - pub id: ShowId, + pub id: ShowIdBuf, pub images: Vec, pub is_externally_hosted: Option, pub languages: Vec, @@ -75,7 +75,7 @@ pub struct SimplifiedEpisode { pub explicit: bool, pub external_urls: HashMap, pub href: String, - pub id: EpisodeId, + pub id: EpisodeIdBuf, pub images: Vec, pub is_externally_hosted: bool, pub is_playable: bool, @@ -100,7 +100,7 @@ pub struct FullEpisode { pub explicit: bool, pub external_urls: HashMap, pub href: String, - pub id: EpisodeId, + pub id: EpisodeIdBuf, pub images: Vec, pub is_externally_hosted: bool, pub is_playable: bool, diff --git a/rspotify-model/src/track.rs b/rspotify-model/src/track.rs index bf924e04..e590d2e7 100644 --- a/rspotify-model/src/track.rs +++ b/rspotify-model/src/track.rs @@ -6,7 +6,8 @@ use serde::{Deserialize, Serialize}; use std::{collections::HashMap, time::Duration}; use crate::{ - custom_serde::duration_ms, Playable, Restriction, SimplifiedAlbum, SimplifiedArtist, TrackId, + custom_serde::duration_ms, PlayableId, Restriction, SimplifiedAlbum, SimplifiedArtist, + TrackIdBuf, }; /// Full track object @@ -24,7 +25,7 @@ pub struct FullTrack { pub external_urls: HashMap, pub href: Option, /// Note that a track may not have an ID/URI if it's local - pub id: Option, + pub id: Option, pub is_local: bool, #[serde(skip_serializing_if = "Option::is_none")] pub is_playable: Option, @@ -43,7 +44,7 @@ pub struct FullTrack { pub struct TrackLink { pub external_urls: HashMap, pub href: String, - pub id: TrackId, + pub id: TrackIdBuf, } /// Intermediate full track wrapped by `Vec` @@ -67,7 +68,7 @@ pub struct SimplifiedTrack { pub external_urls: HashMap, #[serde(default)] pub href: Option, - pub id: Option, + pub id: Option, pub is_local: bool, pub is_playable: Option, pub linked_from: Option, @@ -86,10 +87,10 @@ pub struct SavedTrack { /// Track id with specific positions track in a playlist /// -/// This is a short-lived struct for endpoint parameters, so it uses `&dyn -/// PlayableId` instead of `Box` to avoid the unnecessary -/// allocation. Same goes for the positions slice instead of vector. +/// This is a short-lived struct for endpoint parameters, so it uses `&'a +/// PlayableId` instead of `PlayableIdBuf` to avoid the unnecessary allocation. +/// Same goes for the positions slice instead of vector. pub struct ItemPositions<'a> { - pub id: Playable, + pub id: PlayableId<'a>, pub positions: &'a [u32], } diff --git a/rspotify-model/src/user.rs b/rspotify-model/src/user.rs index 5fec1ed4..1f627ab0 100644 --- a/rspotify-model/src/user.rs +++ b/rspotify-model/src/user.rs @@ -4,7 +4,7 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; -use crate::{Country, Followers, Image, SubscriptionLevel, UserId}; +use crate::{Country, Followers, Image, SubscriptionLevel, UserIdBuf}; /// Public user object #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] @@ -13,7 +13,7 @@ pub struct PublicUser { pub external_urls: HashMap, pub followers: Option, pub href: String, - pub id: UserId, + pub id: UserIdBuf, #[serde(default = "Vec::new")] pub images: Vec, } @@ -28,7 +28,7 @@ pub struct PrivateUser { pub explicit_content: Option, pub followers: Option, pub href: String, - pub id: UserId, + pub id: UserIdBuf, pub images: Option>, pub product: Option, } diff --git a/src/clients/oauth.rs b/src/clients/oauth.rs index 35cfcb9d..1e642909 100644 --- a/src/clients/oauth.rs +++ b/src/clients/oauth.rs @@ -14,7 +14,7 @@ use crate::{ use std::{collections::HashMap, time}; use maybe_async::maybe_async; -use rspotify_model::idtypes::{PlayContext, Playable}; +use rspotify_model::idtypes::{PlayContextId, PlayableId}; use serde_json::{json, Map}; use url::Url; @@ -296,7 +296,7 @@ pub trait OAuthClient: BaseClient { async fn playlist_add_items<'a>( &self, playlist_id: &PlaylistId, - items: impl IntoIterator + Send + 'a, + items: impl IntoIterator> + Send + 'a, position: Option, ) -> ClientResult { let uris = items.into_iter().map(|id| id.uri()).collect::>(); @@ -321,7 +321,7 @@ pub trait OAuthClient: BaseClient { async fn playlist_replace_items<'a>( &self, playlist_id: &PlaylistId, - items: impl IntoIterator + Send + 'a, + items: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let uris = items.into_iter().map(|id| id.uri()).collect::>(); let params = build_json! { @@ -377,7 +377,7 @@ pub trait OAuthClient: BaseClient { async fn playlist_remove_all_occurrences_of_items<'a>( &self, playlist_id: &PlaylistId, - track_ids: impl IntoIterator + Send + 'a, + track_ids: impl IntoIterator> + Send + 'a, snapshot_id: Option<&str>, ) -> ClientResult { let tracks = track_ids @@ -1027,7 +1027,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/start-a-users-playback) async fn start_context_playback( &self, - context_uri: PlayContext, + context_uri: PlayContextId<'_>, device_id: Option<&str>, offset: Option, position_ms: Option, @@ -1059,7 +1059,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/start-a-users-playback) async fn start_uris_playback<'a>( &self, - uris: impl IntoIterator + Send + 'a, + uris: impl IntoIterator> + Send + 'a, device_id: Option<&str>, offset: Option, position_ms: Option, @@ -1220,7 +1220,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/add-to-queue) async fn add_item_to_queue( &self, - item: &Playable, + item: PlayableId<'_>, device_id: Option<&str>, ) -> ClientResult<()> { let url = append_device_id(&format!("me/player/queue?uri={}", item.uri()), device_id); diff --git a/src/lib.rs b/src/lib.rs index cbb9fc2e..0da9dc15 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -151,7 +151,7 @@ use thiserror::Error; pub mod prelude { pub use crate::clients::{BaseClient, OAuthClient}; - pub use crate::model::idtypes::{Id, PlayContext, Playable}; + pub use crate::model::idtypes::{Id, PlayContextId, PlayableId}; } /// Common headers as constants. From cbc1177ed9039e49d5cf79489113c76d4ead1d21 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Fri, 27 May 2022 02:39:56 +0200 Subject: [PATCH 05/28] Almost there --- rspotify-model/src/album.rs | 8 +- rspotify-model/src/artist.rs | 6 +- rspotify-model/src/audio.rs | 4 +- rspotify-model/src/idtypes.rs | 205 +++++++++++---------------------- rspotify-model/src/lib.rs | 7 +- rspotify-model/src/playlist.rs | 6 +- rspotify-model/src/show.rs | 10 +- rspotify-model/src/track.rs | 15 ++- rspotify-model/src/user.rs | 6 +- src/clients/base.rs | 95 +++++++++------ src/clients/oauth.rs | 46 ++++---- src/lib.rs | 20 +++- 12 files changed, 198 insertions(+), 230 deletions(-) diff --git a/rspotify-model/src/album.rs b/rspotify-model/src/album.rs index d4023ae8..f91f641d 100644 --- a/rspotify-model/src/album.rs +++ b/rspotify-model/src/album.rs @@ -6,8 +6,8 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use crate::{ - AlbumIdBuf, AlbumType, Copyright, DatePrecision, Image, Page, RestrictionReason, - SimplifiedArtist, SimplifiedTrack, + AlbumId, AlbumType, Copyright, DatePrecision, Image, Page, RestrictionReason, SimplifiedArtist, + SimplifiedTrack, }; /// Simplified Album Object @@ -21,7 +21,7 @@ pub struct SimplifiedAlbum { pub available_markets: Vec, pub external_urls: HashMap, pub href: Option, - pub id: Option, + pub id: Option>, pub images: Vec, pub name: String, #[serde(skip_serializing_if = "Option::is_none")] @@ -43,7 +43,7 @@ pub struct FullAlbum { pub external_urls: HashMap, pub genres: Vec, pub href: String, - pub id: AlbumIdBuf, + pub id: AlbumId<'static>, pub images: Vec, pub name: String, pub popularity: u32, diff --git a/rspotify-model/src/artist.rs b/rspotify-model/src/artist.rs index a45ee346..2906913e 100644 --- a/rspotify-model/src/artist.rs +++ b/rspotify-model/src/artist.rs @@ -4,14 +4,14 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; -use crate::{ArtistIdBuf, CursorBasedPage, Followers, Image}; +use crate::{ArtistId, CursorBasedPage, Followers, Image}; /// Simplified Artist Object #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Default)] pub struct SimplifiedArtist { pub external_urls: HashMap, pub href: Option, - pub id: Option, + pub id: Option>, pub name: String, } @@ -22,7 +22,7 @@ pub struct FullArtist { pub followers: Followers, pub genres: Vec, pub href: String, - pub id: ArtistIdBuf, + pub id: ArtistId<'static>, pub images: Vec, pub name: String, pub popularity: u32, diff --git a/rspotify-model/src/audio.rs b/rspotify-model/src/audio.rs index 305a971c..b376ac9c 100644 --- a/rspotify-model/src/audio.rs +++ b/rspotify-model/src/audio.rs @@ -6,7 +6,7 @@ use std::time::Duration; use crate::{ custom_serde::{duration_ms, modality}, - Modality, TrackIdBuf, + Modality, TrackId, }; /// Audio Feature Object @@ -18,7 +18,7 @@ pub struct AudioFeatures { #[serde(with = "duration_ms", rename = "duration_ms")] pub duration: Duration, pub energy: f32, - pub id: TrackIdBuf, + pub id: TrackId<'static>, pub instrumentalness: f32, pub key: i32, pub liveness: f32, diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 971a86a2..173c5aac 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -128,7 +128,7 @@ pub enum IdError { /// Unfortunately, since `where Self: Sized` has to be enforced, IDs cannot be /// simply a `str` internally because these aren't sized. Thus, IDs will have the /// slight overhead of having to use an owned type like `String`. -pub trait Id { +pub trait Id<'a> { /// The type of the ID. const TYPE: Type; @@ -145,7 +145,9 @@ pub trait Id { /// /// The string passed to this method must be made out of valid characters /// only; otherwise undefined behaviour may occur. - unsafe fn from_id_unchecked(id: &str) -> &Self; + unsafe fn from_id_unchecked(id: &'a str) -> Self + where + Self: Sized; /// Spotify object URI in a well-known format: `spotify:type:id` /// @@ -171,7 +173,10 @@ pub trait Id { /// # Errors: /// /// - `IdError::InvalidId` - if `id` contains invalid characters. - fn from_id(id: &str) -> Result<&Self, IdError> { + fn from_id(id: &'a str) -> Result + where + Self: Sized, + { if id.chars().all(|ch| ch.is_ascii_alphanumeric()) { // Safe, we've just checked that the Id is valid. Ok(unsafe { Self::from_id_unchecked(id) }) @@ -199,7 +204,10 @@ pub trait Id { /// - `IdError::InvalidId` - if id part of an `uri` is not a valid id, /// - `IdError::InvalidFormat` - if it can't be splitted into type and /// id parts. - fn from_uri(uri: &str) -> Result<&Self, IdError> { + fn from_uri(uri: &'a str) -> Result + where + Self: Sized, + { let mut chars = uri .strip_prefix("spotify") .ok_or(IdError::InvalidPrefix)? @@ -248,7 +256,10 @@ pub trait Id { /// characters), /// - `IdError::InvalidFormat` - if `id_or_uri` is an URI, and it can't be /// split into type and id parts. - fn from_id_or_uri(id_or_uri: &str) -> Result<&Self, IdError> { + fn from_id_or_uri(id_or_uri: &'a str) -> Result + where + Self: Sized, + { match Self::from_uri(id_or_uri) { Ok(id) => Ok(id), Err(IdError::InvalidPrefix) => Self::from_id(id_or_uri), @@ -269,22 +280,31 @@ pub trait Id { /// * The `$name` parameter is the identifier of the struct for that type. macro_rules! define_idtypes { ($($type:ident => { - borrowed: $borrowed:ident, - owned: $owned:ident, + name: $name:ident, validity: $validity:expr }),+) => { $( #[doc = concat!( - "Borrowed ID of type [`Type::", stringify!($type), "`]. Its \ + "ID of type [`Type::", stringify!($type), "`]. Its \ implementation of `id_is_valid` is defined by the closure `", stringify!($validity), "`.\nRefer to the [module-level \ docs][`crate::idtypes`] for more information. " )] #[repr(transparent)] - #[derive(Debug, PartialEq, Eq, Serialize, Hash)] - pub struct $borrowed(str); + #[derive(Clone, Debug, PartialEq, Eq, Serialize, Hash)] + pub struct $name<'a>(::std::borrow::Cow<'a, str>); - impl Id for $borrowed { + impl<'a> $name<'a> { + pub fn into_static(self) -> $name<'static> { + $name(self.0.to_string().into()) + } + + pub fn clone_static(&self) -> $name<'static> { + $name(self.0.to_string().into()) + } + } + + impl<'a> Id<'a> for $name<'a> { const TYPE: Type = Type::$type; #[inline] @@ -300,30 +320,14 @@ macro_rules! define_idtypes { } #[inline] - unsafe fn from_id_unchecked(id: &str) -> &Self { - // SAFETY: this is okay because Ids are transparent over - // their inner type, a `str`. It would be equivalent to a - // `transmute`. See: - // https://doc.rust-lang.org/nomicon/casts.html - &*(id as *const str as *const Self) + unsafe fn from_id_unchecked(id: &'a str) -> Self { + Self(std::borrow::Cow::Borrowed(id)) } } - #[doc = concat!( - "Owned ID of type [`Type::", stringify!($type), "`], made up \ - of only alphanumeric characters. Note that we don't really \ - need to change the contents of the URI string, so we can use a \ - `Box` instead of a `String` for its owned variant. Not \ - only does this make it easier to implement and use, but also \ - it reduces its size (on my machine it's 16 bytes vs 24 bytes). \ - Refer to the [module-level docs][`crate::idtypes`] for more \ - information." - )] - pub type $owned = Box<$borrowed>; - // Deserialization may take either an Id or an URI, so its // implementation has to be done manually. - impl<'de> Deserialize<'de> for $owned { + impl<'de> Deserialize<'de> for $name<'static> { fn deserialize(deserializer: D) -> Result where D: serde::Deserializer<'de>, @@ -331,7 +335,7 @@ macro_rules! define_idtypes { struct IdVisitor; impl<'de> serde::de::Visitor<'de> for IdVisitor { - type Value = $owned; + type Value = $name<'static>; fn expecting( &self, formatter: &mut std::fmt::Formatter<'_> @@ -346,8 +350,8 @@ macro_rules! define_idtypes { where E: serde::de::Error, { - $borrowed::from_id_or_uri(value) - .map(ToOwned::to_owned) + $name::from_id_or_uri(value) + .map($name::into_static) .map_err(serde::de::Error::custom) } @@ -372,8 +376,8 @@ macro_rules! define_idtypes { { let field = seq.next_element()? .ok_or_else(|| serde::de::Error::invalid_length(0, &self))?; - $borrowed::from_id_or_uri(field) - .map(ToOwned::to_owned) + $name::from_id_or_uri(field) + .map($name::into_static) .map_err(serde::de::Error::custom) } } @@ -383,7 +387,7 @@ macro_rules! define_idtypes { } /// Cheap conversion to `str` - impl AsRef for $borrowed { + impl AsRef for $name<'_> { fn as_ref(&self) -> &str { self.id() } @@ -391,55 +395,18 @@ macro_rules! define_idtypes { /// `Id`s may be borrowed as `str` the same way `Box` may be /// borrowed as `T` or `String` as `str` - impl std::borrow::Borrow for $borrowed { + impl std::borrow::Borrow for $name<'_> { fn borrow(&self) -> &str { self.id() } } /// Displaying the ID shows its URI - impl std::fmt::Display for $borrowed { + impl std::fmt::Display for $name<'_> { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { write!(f, "{}", self.uri()) } } - - impl std::borrow::ToOwned for $borrowed { - type Owned = $owned; - - fn to_owned(&self) -> Self::Owned { - todo!() - } - } - - /// `Box` implements clone, but that doesn't cover `Box` - /// where `T` is a newtype for `str`. Thus, this manually delegates - /// `Clone` for `Box` to the implementation for `Box`. - impl Clone for $owned { - fn clone(&self) -> $owned { - // SAFETY: this is okay because Ids are transparent over - // their inner type, a `str`. See: - // https://doc.rust-lang.org/stable/nomicon/transmutes.html - let id: &Box = unsafe { std::mem::transmute(self) }; - - // Calling `Box::clone`, which is implemented - let id = id.clone(); - - // SAFETY: same as above, but in the inverse direction. - unsafe { std::mem::transmute(id) } - } - } - - /// Exactly the same thing in `Clone` happens with `Default`. - impl Default for $owned { - fn default() -> $owned { - // Calling `Box::default`, which is implemented - let id: Box = Default::default(); - - // SAFETY: same as in `Clone`. - unsafe { std::mem::transmute(id) } - } - } )+ } } @@ -448,105 +415,71 @@ macro_rules! define_idtypes { // declared manually later on. define_idtypes!( Artist => { - borrowed: ArtistId, - owned: ArtistIdBuf, + name: ArtistId, validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) }, Album => { - borrowed: AlbumId, - owned: AlbumIdBuf, + name: AlbumId, validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) }, Track => { - borrowed: TrackId, - owned: TrackIdBuf, + name: TrackId, validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) }, Playlist => { - borrowed: PlaylistId, - owned: PlaylistIdBuf, + name: PlaylistId, validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) }, Show => { - borrowed: ShowId, - owned: ShowIdBuf, + name: ShowId, validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) }, Episode => { - borrowed: EpisodeId, - owned: EpisodeIdBuf, + name: EpisodeId, validity: |id| id.chars().all(|ch| ch.is_ascii_alphanumeric()) }, User => { - borrowed: UserId, - owned: UserIdBuf, + name: UserId, validity: |_| true } ); +// TODO: replace with `enum_dispatch`? macro_rules! define_idgroups { - ($(pub enum ($borrowed:ident, $owned:ident) { + ($(pub enum $name:ident { $( - $variant_name:ident => ($variant_borrowed:ident, $variant_owned:ident) + $variant_name:ident($variant_type:ident) ),+ + $(,)? }),+) => { $( - pub enum $borrowed<'a> { - $( - $variant_name(&'a $variant_borrowed), - )+ - } - - pub enum $owned { + pub enum $name<'a> { $( - $variant_name($variant_owned), + $variant_name($variant_type<'a>), )+ } - - impl<'a> $borrowed<'a> { - pub fn uri(&self) -> String { - match self { - $( - $borrowed::$variant_name(x) => x.uri(), - )+ - } - } - pub fn url(&self) -> String { - match self { - $( - $borrowed::$variant_name(x) => x.url(), - )+ - } - } - pub const fn _type(&self) -> Type { - match self { - $( - $borrowed::$variant_name(_) => $variant_borrowed::TYPE, - )+ - } - } - } - - impl $owned { + // TODO: turn into `impl Id`? + // TODO: also implement `into_owned` and etc + impl<'a> $name<'a> { pub fn uri(&self) -> String { match self { $( - $owned::$variant_name(x) => x.uri(), + $name::$variant_name(x) => x.uri(), )+ } } pub fn url(&self) -> String { match self { $( - $owned::$variant_name(x) => x.url(), + $name::$variant_name(x) => x.url(), )+ } } pub const fn _type(&self) -> Type { match self { $( - $owned::$variant_name(_) => $variant_borrowed::TYPE, + $name::$variant_name(_) => $variant_type::TYPE, )+ } } @@ -557,15 +490,15 @@ macro_rules! define_idgroups { // Grouping up the IDs into more specific traits define_idgroups!( - pub enum (PlayContextId, PlayContextIdBuf) { - Artist => (ArtistId, ArtistIdBuf), - Album => (AlbumId, AlbumIdBuf), - Playlist => (PlaylistId, PlaylistIdBuf), - Show => (ShowId, ShowIdBuf) + pub enum PlayContextId { + Artist(ArtistId), + Album(AlbumId), + Playlist(PlaylistId), + Show(ShowId), }, - pub enum (PlayableId, PlayableIdBuf) { - Track => (TrackId, TrackIdBuf), - Episode => (EpisodeId, EpisodeIdBuf) + pub enum PlayableId { + Track(TrackId), + Episode(EpisodeId), } ); diff --git a/rspotify-model/src/lib.rs b/rspotify-model/src/lib.rs index 49ada28a..07c9a61a 100644 --- a/rspotify-model/src/lib.rs +++ b/rspotify-model/src/lib.rs @@ -53,10 +53,11 @@ impl PlayableItem { /// /// Note that if it's a track and if it's local, it may not have an ID, in /// which case this function will return `None`. - pub fn id(&self) -> Option> { + pub fn id(&self) -> Option> { match self { - PlayableItem::Track(t) => t.id.as_ref().map(|id| PlayableId::Track(id)), - PlayableItem::Episode(e) => Some(PlayableId::Episode(&e.id)), + // TODO: can we avoid `clone` here and return `PlayableId<'_>`? + PlayableItem::Track(t) => t.id.clone().map(PlayableId::Track), + PlayableItem::Episode(e) => Some(PlayableId::Episode(e.id.clone())), } } } diff --git a/rspotify-model/src/playlist.rs b/rspotify-model/src/playlist.rs index b220d8d8..1a8fa36a 100644 --- a/rspotify-model/src/playlist.rs +++ b/rspotify-model/src/playlist.rs @@ -5,7 +5,7 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; -use crate::{Followers, Image, Page, PlayableItem, PlaylistIdBuf, PublicUser}; +use crate::{Followers, Image, Page, PlayableItem, PlaylistId, PublicUser}; /// Playlist result object #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Default)] @@ -26,7 +26,7 @@ pub struct SimplifiedPlaylist { pub collaborative: bool, pub external_urls: HashMap, pub href: String, - pub id: PlaylistIdBuf, + pub id: PlaylistId<'static>, pub images: Vec, pub name: String, pub owner: PublicUser, @@ -43,7 +43,7 @@ pub struct FullPlaylist { pub external_urls: HashMap, pub followers: Followers, pub href: String, - pub id: PlaylistIdBuf, + pub id: PlaylistId<'static>, pub images: Vec, pub name: String, pub owner: PublicUser, diff --git a/rspotify-model/src/show.rs b/rspotify-model/src/show.rs index d64a2ca1..f9a89ff0 100644 --- a/rspotify-model/src/show.rs +++ b/rspotify-model/src/show.rs @@ -4,7 +4,7 @@ use std::collections::HashMap; use std::time::Duration; use crate::{ - custom_serde::duration_ms, CopyrightType, DatePrecision, EpisodeIdBuf, Image, Page, ShowIdBuf, + custom_serde::duration_ms, CopyrightType, DatePrecision, EpisodeId, Image, Page, ShowId, }; /// Copyright object @@ -24,7 +24,7 @@ pub struct SimplifiedShow { pub explicit: bool, pub external_urls: HashMap, pub href: String, - pub id: ShowIdBuf, + pub id: ShowId<'static>, pub images: Vec, pub is_externally_hosted: Option, pub languages: Vec, @@ -56,7 +56,7 @@ pub struct FullShow { pub episodes: Page, pub external_urls: HashMap, pub href: String, - pub id: ShowIdBuf, + pub id: ShowId<'static>, pub images: Vec, pub is_externally_hosted: Option, pub languages: Vec, @@ -75,7 +75,7 @@ pub struct SimplifiedEpisode { pub explicit: bool, pub external_urls: HashMap, pub href: String, - pub id: EpisodeIdBuf, + pub id: EpisodeId<'static>, pub images: Vec, pub is_externally_hosted: bool, pub is_playable: bool, @@ -100,7 +100,7 @@ pub struct FullEpisode { pub explicit: bool, pub external_urls: HashMap, pub href: String, - pub id: EpisodeIdBuf, + pub id: EpisodeId<'static>, pub images: Vec, pub is_externally_hosted: bool, pub is_playable: bool, diff --git a/rspotify-model/src/track.rs b/rspotify-model/src/track.rs index e590d2e7..dbcd7696 100644 --- a/rspotify-model/src/track.rs +++ b/rspotify-model/src/track.rs @@ -6,8 +6,7 @@ use serde::{Deserialize, Serialize}; use std::{collections::HashMap, time::Duration}; use crate::{ - custom_serde::duration_ms, PlayableId, Restriction, SimplifiedAlbum, SimplifiedArtist, - TrackIdBuf, + custom_serde::duration_ms, PlayableId, Restriction, SimplifiedAlbum, SimplifiedArtist, TrackId, }; /// Full track object @@ -25,7 +24,7 @@ pub struct FullTrack { pub external_urls: HashMap, pub href: Option, /// Note that a track may not have an ID/URI if it's local - pub id: Option, + pub id: Option>, pub is_local: bool, #[serde(skip_serializing_if = "Option::is_none")] pub is_playable: Option, @@ -44,7 +43,7 @@ pub struct FullTrack { pub struct TrackLink { pub external_urls: HashMap, pub href: String, - pub id: TrackIdBuf, + pub id: TrackId<'static>, } /// Intermediate full track wrapped by `Vec` @@ -68,7 +67,7 @@ pub struct SimplifiedTrack { pub external_urls: HashMap, #[serde(default)] pub href: Option, - pub id: Option, + pub id: Option>, pub is_local: bool, pub is_playable: Option, pub linked_from: Option, @@ -87,9 +86,9 @@ pub struct SavedTrack { /// Track id with specific positions track in a playlist /// -/// This is a short-lived struct for endpoint parameters, so it uses `&'a -/// PlayableId` instead of `PlayableIdBuf` to avoid the unnecessary allocation. -/// Same goes for the positions slice instead of vector. +/// This is a short-lived struct for endpoint parameters, so it uses +/// `PlayableId<'a>` instead of `PlayableId<'static>` to avoid the unnecessary +/// allocation. Same goes for the positions slice instead of vector. pub struct ItemPositions<'a> { pub id: PlayableId<'a>, pub positions: &'a [u32], diff --git a/rspotify-model/src/user.rs b/rspotify-model/src/user.rs index 1f627ab0..86978760 100644 --- a/rspotify-model/src/user.rs +++ b/rspotify-model/src/user.rs @@ -4,7 +4,7 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; -use crate::{Country, Followers, Image, SubscriptionLevel, UserIdBuf}; +use crate::{Country, Followers, Image, SubscriptionLevel, UserId}; /// Public user object #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] @@ -13,7 +13,7 @@ pub struct PublicUser { pub external_urls: HashMap, pub followers: Option, pub href: String, - pub id: UserIdBuf, + pub id: UserId<'static>, #[serde(default = "Vec::new")] pub images: Vec, } @@ -28,7 +28,7 @@ pub struct PrivateUser { pub explicit_content: Option, pub followers: Option, pub href: String, - pub id: UserIdBuf, + pub id: UserId<'static>, pub images: Option>, pub product: Option, } diff --git a/src/clients/base.rs b/src/clients/base.rs index c5e6e9e5..bdc03cf0 100644 --- a/src/clients/base.rs +++ b/src/clients/base.rs @@ -244,7 +244,7 @@ where /// - track_id - a spotify URI, URL or ID /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-track) - async fn track(&self, track_id: &TrackId) -> ClientResult { + async fn track(&self, track_id: TrackId<'_>) -> ClientResult { let url = format!("tracks/{}", track_id.id()); let result = self.endpoint_get(&url, &Query::new()).await?; convert_result(&result) @@ -259,7 +259,7 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-several-tracks) async fn tracks<'a>( &self, - track_ids: impl IntoIterator + Send + 'a, + track_ids: impl IntoIterator> + Send + 'a, market: Option<&Market>, ) -> ClientResult> { let ids = join_ids(track_ids); @@ -278,7 +278,7 @@ where /// - artist_id - an artist ID, URI or URL /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-artist) - async fn artist(&self, artist_id: &ArtistId) -> ClientResult { + async fn artist(&self, artist_id: ArtistId<'_>) -> ClientResult { let url = format!("artists/{}", artist_id.id()); let result = self.endpoint_get(&url, &Query::new()).await?; convert_result(&result) @@ -292,7 +292,7 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-multiple-artists) async fn artists<'a>( &self, - artist_ids: impl IntoIterator + Send + 'a, + artist_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult> { let ids = join_ids(artist_ids); let url = format!("artists/?ids={}", ids); @@ -316,13 +316,19 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-artists-albums) fn artist_albums<'a>( &'a self, - artist_id: &'a ArtistId, + artist_id: ArtistId<'a>, album_type: Option<&'a AlbumType>, market: Option<&'a Market>, ) -> Paginator<'_, ClientResult> { paginate( move |limit, offset| { - self.artist_albums_manual(artist_id, album_type, market, Some(limit), Some(offset)) + self.artist_albums_manual( + artist_id.clone(), + album_type, + market, + Some(limit), + Some(offset), + ) }, self.get_config().pagination_chunks, ) @@ -331,7 +337,7 @@ where /// The manually paginated version of [`Self::artist_albums`]. async fn artist_albums_manual( &self, - artist_id: &ArtistId, + artist_id: ArtistId<'_>, album_type: Option<&AlbumType>, market: Option<&Market>, limit: Option, @@ -361,7 +367,7 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-artists-top-tracks) async fn artist_top_tracks( &self, - artist_id: &ArtistId, + artist_id: ArtistId<'_>, market: &Market, ) -> ClientResult> { let params = build_map! { @@ -381,7 +387,10 @@ where /// - artist_id - the artist ID, URI or URL /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-artists-related-artists) - async fn artist_related_artists(&self, artist_id: &ArtistId) -> ClientResult> { + async fn artist_related_artists( + &self, + artist_id: ArtistId<'_>, + ) -> ClientResult> { let url = format!("artists/{}/related-artists", artist_id.id()); let result = self.endpoint_get(&url, &Query::new()).await?; convert_result::(&result).map(|x| x.artists) @@ -393,7 +402,7 @@ where /// - album_id - the album ID, URI or URL /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-album) - async fn album(&self, album_id: &AlbumId) -> ClientResult { + async fn album(&self, album_id: AlbumId<'_>) -> ClientResult { let url = format!("albums/{}", album_id.id()); let result = self.endpoint_get(&url, &Query::new()).await?; @@ -408,7 +417,7 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-multiple-albums) async fn albums<'a>( &self, - album_ids: impl IntoIterator + Send + 'a, + album_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult> { let ids = join_ids(album_ids); let url = format!("albums/?ids={}", ids); @@ -468,10 +477,12 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-albums-tracks) fn album_track<'a>( &'a self, - album_id: &'a AlbumId, + album_id: AlbumId<'a>, ) -> Paginator<'_, ClientResult> { paginate( - move |limit, offset| self.album_track_manual(album_id, Some(limit), Some(offset)), + move |limit, offset| { + self.album_track_manual(album_id.clone(), Some(limit), Some(offset)) + }, self.get_config().pagination_chunks, ) } @@ -479,7 +490,7 @@ where /// The manually paginated version of [`Self::album_track`]. async fn album_track_manual( &self, - album_id: &AlbumId, + album_id: AlbumId<'_>, limit: Option, offset: Option, ) -> ClientResult> { @@ -501,7 +512,7 @@ where /// - user - the id of the usr /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-users-profile) - async fn user(&self, user_id: &UserId) -> ClientResult { + async fn user(&self, user_id: UserId<'_>) -> ClientResult { let url = format!("users/{}", user_id.id()); let result = self.endpoint_get(&url, &Query::new()).await?; convert_result(&result) @@ -516,7 +527,7 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-playlist) async fn playlist( &self, - playlist_id: &PlaylistId, + playlist_id: PlaylistId<'_>, fields: Option<&str>, market: Option<&Market>, ) -> ClientResult { @@ -540,8 +551,8 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-list-users-playlists) async fn user_playlist( &self, - user_id: &UserId, - playlist_id: Option<&PlaylistId>, + user_id: UserId<'_>, + playlist_id: Option>, fields: Option<&str>, ) -> ClientResult { let params = build_map! { @@ -566,8 +577,8 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/check-if-user-follows-playlist) async fn playlist_check_follow( &self, - playlist_id: &PlaylistId, - user_ids: &[&UserId], + playlist_id: PlaylistId<'_>, + user_ids: &[UserId<'_>], ) -> ClientResult> { debug_assert!( user_ids.len() <= 5, @@ -595,7 +606,7 @@ where /// - market(Optional): An ISO 3166-1 alpha-2 country code or the string from_token. /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-a-show) - async fn get_a_show(&self, id: &ShowId, market: Option<&Market>) -> ClientResult { + async fn get_a_show(&self, id: ShowId<'_>, market: Option<&Market>) -> ClientResult { let params = build_map! { optional "market": market.map(|x| x.as_ref()), }; @@ -615,7 +626,7 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-multiple-shows) async fn get_several_shows<'a>( &self, - ids: impl IntoIterator + Send + 'a, + ids: impl IntoIterator> + Send + 'a, market: Option<&Market>, ) -> ClientResult> { let ids = join_ids(ids); @@ -645,12 +656,12 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-a-shows-episodes) fn get_shows_episodes<'a>( &'a self, - id: &'a ShowId, + id: ShowId<'a>, market: Option<&'a Market>, ) -> Paginator<'_, ClientResult> { paginate( move |limit, offset| { - self.get_shows_episodes_manual(id, market, Some(limit), Some(offset)) + self.get_shows_episodes_manual(id.clone(), market, Some(limit), Some(offset)) }, self.get_config().pagination_chunks, ) @@ -659,7 +670,7 @@ where /// The manually paginated version of [`Self::get_shows_episodes`]. async fn get_shows_episodes_manual( &self, - id: &ShowId, + id: ShowId<'_>, market: Option<&Market>, limit: Option, offset: Option, @@ -688,7 +699,7 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-episode) async fn get_an_episode( &self, - id: &EpisodeId, + id: EpisodeId<'_>, market: Option<&Market>, ) -> ClientResult { let url = format!("episodes/{}", id.id()); @@ -709,7 +720,7 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-multiple-episodes) async fn get_several_episodes<'a>( &self, - ids: impl IntoIterator + Send + 'a, + ids: impl IntoIterator> + Send + 'a, market: Option<&Market>, ) -> ClientResult> { let ids = join_ids(ids); @@ -728,7 +739,7 @@ where /// - track - track URI, URL or ID /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-audio-features) - async fn track_features(&self, track_id: &TrackId) -> ClientResult { + async fn track_features(&self, track_id: TrackId<'_>) -> ClientResult { let url = format!("audio-features/{}", track_id.id()); let result = self.endpoint_get(&url, &Query::new()).await?; convert_result(&result) @@ -742,7 +753,7 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-several-audio-features) async fn tracks_features<'a>( &self, - track_ids: impl IntoIterator + Send + 'a, + track_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult>> { let url = format!("audio-features/?ids={}", join_ids(track_ids)); @@ -761,7 +772,7 @@ where /// - track_id - a track URI, URL or ID /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-audio-analysis) - async fn track_analysis(&self, track_id: &TrackId) -> ClientResult { + async fn track_analysis(&self, track_id: TrackId<'_>) -> ClientResult { let url = format!("audio-analysis/{}", track_id.id()); let result = self.endpoint_get(&url, &Query::new()).await?; convert_result(&result) @@ -965,9 +976,9 @@ where async fn recommendations<'a>( &self, attributes: impl IntoIterator + Send + 'a, - seed_artists: Option + Send + 'a>, + seed_artists: Option> + Send + 'a>, seed_genres: Option + Send + 'a>, - seed_tracks: Option + Send + 'a>, + seed_tracks: Option> + Send + 'a>, market: Option<&Market>, limit: Option, ) -> ClientResult { @@ -1015,13 +1026,19 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-playlists-tracks) fn playlist_items<'a>( &'a self, - playlist_id: &'a PlaylistId, + playlist_id: PlaylistId<'a>, fields: Option<&'a str>, market: Option<&'a Market>, ) -> Paginator<'_, ClientResult> { paginate( move |limit, offset| { - self.playlist_items_manual(playlist_id, fields, market, Some(limit), Some(offset)) + self.playlist_items_manual( + playlist_id.clone(), + fields, + market, + Some(limit), + Some(offset), + ) }, self.get_config().pagination_chunks, ) @@ -1030,7 +1047,7 @@ where /// The manually paginated version of [`Self::playlist_items`]. async fn playlist_items_manual( &self, - playlist_id: &PlaylistId, + playlist_id: PlaylistId<'_>, fields: Option<&str>, market: Option<&Market>, limit: Option, @@ -1063,10 +1080,12 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-list-users-playlists) fn user_playlists<'a>( &'a self, - user_id: &'a UserId, + user_id: UserId<'a>, ) -> Paginator<'_, ClientResult> { paginate( - move |limit, offset| self.user_playlists_manual(user_id, Some(limit), Some(offset)), + move |limit, offset| { + self.user_playlists_manual(user_id.clone(), Some(limit), Some(offset)) + }, self.get_config().pagination_chunks, ) } @@ -1074,7 +1093,7 @@ where /// The manually paginated version of [`Self::user_playlists`]. async fn user_playlists_manual( &self, - user_id: &UserId, + user_id: UserId<'_>, limit: Option, offset: Option, ) -> ClientResult> { diff --git a/src/clients/oauth.rs b/src/clients/oauth.rs index 1e642909..2d352966 100644 --- a/src/clients/oauth.rs +++ b/src/clients/oauth.rs @@ -225,7 +225,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/create-playlist) async fn user_playlist_create( &self, - user_id: &UserId, + user_id: UserId<'_>, name: &str, public: Option, collaborative: Option, @@ -255,7 +255,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/change-playlist-details) async fn playlist_change_detail( &self, - playlist_id: &PlaylistId, + playlist_id: PlaylistId<'_>, name: Option<&str>, public: Option, description: Option<&str>, @@ -278,7 +278,7 @@ pub trait OAuthClient: BaseClient { /// - playlist_id - the id of the playlist /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/unfollow-playlist) - async fn playlist_unfollow(&self, playlist_id: &PlaylistId) -> ClientResult<()> { + async fn playlist_unfollow(&self, playlist_id: PlaylistId<'_>) -> ClientResult<()> { let url = format!("playlists/{}/followers", playlist_id.id()); self.endpoint_delete(&url, &json!({})).await?; @@ -295,7 +295,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/add-tracks-to-playlist) async fn playlist_add_items<'a>( &self, - playlist_id: &PlaylistId, + playlist_id: PlaylistId<'_>, items: impl IntoIterator> + Send + 'a, position: Option, ) -> ClientResult { @@ -320,7 +320,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/reorder-or-replace-playlists-tracks) async fn playlist_replace_items<'a>( &self, - playlist_id: &PlaylistId, + playlist_id: PlaylistId<'_>, items: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let uris = items.into_iter().map(|id| id.uri()).collect::>(); @@ -348,7 +348,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/reorder-or-replace-playlists-tracks) async fn playlist_reorder_items( &self, - playlist_id: &PlaylistId, + playlist_id: PlaylistId<'_>, range_start: Option, insert_before: Option, range_length: Option, @@ -376,7 +376,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/remove-tracks-playlist) async fn playlist_remove_all_occurrences_of_items<'a>( &self, - playlist_id: &PlaylistId, + playlist_id: PlaylistId<'_>, track_ids: impl IntoIterator> + Send + 'a, snapshot_id: Option<&str>, ) -> ClientResult { @@ -430,7 +430,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/remove-tracks-playlist) async fn playlist_remove_specific_occurrences_of_items<'a>( &self, - playlist_id: &PlaylistId, + playlist_id: PlaylistId<'_>, items: impl IntoIterator> + Send + 'a, snapshot_id: Option<&str>, ) -> ClientResult { @@ -462,7 +462,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/follow-playlist) async fn playlist_follow( &self, - playlist_id: &PlaylistId, + playlist_id: PlaylistId<'_>, public: Option, ) -> ClientResult<()> { let url = format!("playlists/{}/followers", playlist_id.id()); @@ -624,7 +624,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/remove-tracks-user) async fn current_user_saved_tracks_delete<'a>( &self, - track_ids: impl IntoIterator + Send + 'a, + track_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let url = format!("me/tracks/?ids={}", join_ids(track_ids)); self.endpoint_delete(&url, &json!({})).await?; @@ -641,7 +641,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/check-users-saved-tracks) async fn current_user_saved_tracks_contains<'a>( &self, - track_ids: impl IntoIterator + Send + 'a, + track_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult> { let url = format!("me/tracks/contains/?ids={}", join_ids(track_ids)); let result = self.endpoint_get(&url, &Query::new()).await?; @@ -656,7 +656,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/save-tracks-user) async fn current_user_saved_tracks_add<'a>( &self, - track_ids: impl IntoIterator + Send + 'a, + track_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let url = format!("me/tracks/?ids={}", join_ids(track_ids)); self.endpoint_put(&url, &json!({})).await?; @@ -789,7 +789,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/save-albums-user) async fn current_user_saved_albums_add<'a>( &self, - album_ids: impl IntoIterator + Send + 'a, + album_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let url = format!("me/albums/?ids={}", join_ids(album_ids)); self.endpoint_put(&url, &json!({})).await?; @@ -805,7 +805,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/remove-albums-user) async fn current_user_saved_albums_delete<'a>( &self, - album_ids: impl IntoIterator + Send + 'a, + album_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let url = format!("me/albums/?ids={}", join_ids(album_ids)); self.endpoint_delete(&url, &json!({})).await?; @@ -822,7 +822,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/check-users-saved-albums) async fn current_user_saved_albums_contains<'a>( &self, - album_ids: impl IntoIterator + Send + 'a, + album_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult> { let url = format!("me/albums/contains/?ids={}", join_ids(album_ids)); let result = self.endpoint_get(&url, &Query::new()).await?; @@ -837,7 +837,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/follow-artists-users) async fn user_follow_artists<'a>( &self, - artist_ids: impl IntoIterator + Send + 'a, + artist_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let url = format!("me/following?type=artist&ids={}", join_ids(artist_ids)); self.endpoint_put(&url, &json!({})).await?; @@ -853,7 +853,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/unfollow-artists-users) async fn user_unfollow_artists<'a>( &self, - artist_ids: impl IntoIterator + Send + 'a, + artist_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let url = format!("me/following?type=artist&ids={}", join_ids(artist_ids)); self.endpoint_delete(&url, &json!({})).await?; @@ -870,7 +870,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/check-current-user-follows) async fn user_artist_check_follow<'a>( &self, - artist_ids: impl IntoIterator + Send + 'a, + artist_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult> { let url = format!( "me/following/contains?type=artist&ids={}", @@ -888,7 +888,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/follow-artists-users) async fn user_follow_users<'a>( &self, - user_ids: impl IntoIterator + Send + 'a, + user_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let url = format!("me/following?type=user&ids={}", join_ids(user_ids)); self.endpoint_put(&url, &json!({})).await?; @@ -904,7 +904,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/unfollow-artists-users) async fn user_unfollow_users<'a>( &self, - user_ids: impl IntoIterator + Send + 'a, + user_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let url = format!("me/following?type=user&ids={}", join_ids(user_ids)); self.endpoint_delete(&url, &json!({})).await?; @@ -1238,7 +1238,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/save-shows-user) async fn save_shows<'a>( &self, - show_ids: impl IntoIterator + Send + 'a, + show_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult<()> { let url = format!("me/shows/?ids={}", join_ids(show_ids)); self.endpoint_put(&url, &json!({})).await?; @@ -1291,7 +1291,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/check-users-saved-shows) async fn check_users_saved_shows<'a>( &self, - ids: impl IntoIterator + Send + 'a, + ids: impl IntoIterator> + Send + 'a, ) -> ClientResult> { let ids = join_ids(ids); let params = build_map! { @@ -1311,7 +1311,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/remove-shows-user) async fn remove_users_saved_shows<'a>( &self, - show_ids: impl IntoIterator + Send + 'a, + show_ids: impl IntoIterator> + Send + 'a, country: Option<&Market>, ) -> ClientResult<()> { let url = format!("me/shows?ids={}", join_ids(show_ids)); diff --git a/src/lib.rs b/src/lib.rs index 0da9dc15..adcf145b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -285,9 +285,25 @@ pub(in crate) fn generate_random_string(length: usize, alphabet: &[u8]) -> Strin .collect() } +// TODO: is it possible to take a `IntoIterator` instead of a +// `IntoIterator`? Now we have `Id<'a>` instead of `&'a T`, so +// technically `IntoIterator` is the same as `IntoIterator>`. Otherwise, this function is taking a double reference, and requires +// more boilerplate. +// +// However, this seems to be impossible because then the function would own the +// Ids, and then we can't call `Id::id`. +// +// Hack: turning this into a macro (best if avoided). #[inline] -pub(in crate) fn join_ids<'a, T: Id + 'a + ?Sized>(ids: impl IntoIterator) -> String { - ids.into_iter().map(Id::id).collect::>().join(",") +pub(in crate) fn join_ids<'a, T: Id<'a> + 'a>(ids: impl IntoIterator) -> String { + // Nope + // ids.into_iter().map(Id::id).collect::>().join(",") + + // Nope + // ids.into_iter().map(|x| x.id()).collect::>().join(",") + + todo!() } #[inline] From 0f6d43963f063946c081b0bfaf2285518809e35f Mon Sep 17 00:00:00 2001 From: eladyn Date: Sat, 28 May 2022 16:05:12 +0200 Subject: [PATCH 06/28] introduce as_borrowed() for id types --- rspotify-model/src/idtypes.rs | 14 ++++++++++++++ rspotify-model/src/lib.rs | 7 +++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 173c5aac..703ee2f8 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -401,6 +401,12 @@ macro_rules! define_idtypes { } } + impl<'a> $name<'a> { + pub fn as_borrowed(&'a self) -> $name<'a> { + Self(std::borrow::Cow::Borrowed(self.0.as_ref())) + } + } + /// Displaying the ID shows its URI impl std::fmt::Display for $name<'_> { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { @@ -483,6 +489,14 @@ macro_rules! define_idgroups { )+ } } + + pub fn as_borrowed(&'a self) -> Self { + match self { + $( + $name::$variant_name(x) => $name::$variant_name(x.as_borrowed()), + )+ + } + } } )+ } diff --git a/rspotify-model/src/lib.rs b/rspotify-model/src/lib.rs index 07c9a61a..d1b18134 100644 --- a/rspotify-model/src/lib.rs +++ b/rspotify-model/src/lib.rs @@ -53,11 +53,10 @@ impl PlayableItem { /// /// Note that if it's a track and if it's local, it may not have an ID, in /// which case this function will return `None`. - pub fn id(&self) -> Option> { + pub fn id(&self) -> Option> { match self { - // TODO: can we avoid `clone` here and return `PlayableId<'_>`? - PlayableItem::Track(t) => t.id.clone().map(PlayableId::Track), - PlayableItem::Episode(e) => Some(PlayableId::Episode(e.id.clone())), + PlayableItem::Track(t) => t.id.as_ref().map(|t| PlayableId::Track(t.as_borrowed())), + PlayableItem::Episode(e) => Some(PlayableId::Episode(e.id.as_borrowed())), } } } From e07a0ed107915c1033e9178b162656ad21cf52b6 Mon Sep 17 00:00:00 2001 From: eladyn Date: Sat, 28 May 2022 16:06:52 +0200 Subject: [PATCH 07/28] fix tests --- rspotify-model/src/idtypes.rs | 2 +- src/clients/base.rs | 4 +- src/lib.rs | 14 ++-- tests/test_with_credential.rs | 42 ++++++------ tests/test_with_oauth.rs | 124 +++++++++++++++++----------------- 5 files changed, 93 insertions(+), 93 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 703ee2f8..1626d8d0 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -177,7 +177,7 @@ pub trait Id<'a> { where Self: Sized, { - if id.chars().all(|ch| ch.is_ascii_alphanumeric()) { + if Self::id_is_valid(id) { // Safe, we've just checked that the Id is valid. Ok(unsafe { Self::from_id_unchecked(id) }) } else { diff --git a/src/clients/base.rs b/src/clients/base.rs index bdc03cf0..cff966a9 100644 --- a/src/clients/base.rs +++ b/src/clients/base.rs @@ -1026,14 +1026,14 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-playlists-tracks) fn playlist_items<'a>( &'a self, - playlist_id: PlaylistId<'a>, + playlist_id: &'a PlaylistId<'_>, fields: Option<&'a str>, market: Option<&'a Market>, ) -> Paginator<'_, ClientResult> { paginate( move |limit, offset| { self.playlist_items_manual( - playlist_id.clone(), + playlist_id.as_borrowed(), fields, market, Some(limit), diff --git a/src/lib.rs b/src/lib.rs index adcf145b..1f86917d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -297,13 +297,13 @@ pub(in crate) fn generate_random_string(length: usize, alphabet: &[u8]) -> Strin // Hack: turning this into a macro (best if avoided). #[inline] pub(in crate) fn join_ids<'a, T: Id<'a> + 'a>(ids: impl IntoIterator) -> String { - // Nope - // ids.into_iter().map(Id::id).collect::>().join(",") - - // Nope - // ids.into_iter().map(|x| x.id()).collect::>().join(",") - - todo!() + let mut ids = ids.into_iter(); + let mut joined = if let Some(first) = ids.next() { String::from(first.id()) } else { return String::new() }; + for id in ids { + joined.push_str(","); + joined.push_str(id.id()); + } + joined } #[inline] diff --git a/tests/test_with_credential.rs b/tests/test_with_credential.rs index 77968853..151a3968 100644 --- a/tests/test_with_credential.rs +++ b/tests/test_with_credential.rs @@ -26,15 +26,15 @@ pub async fn creds_client() -> ClientCredsSpotify { #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] async fn test_album() { let birdy_uri = AlbumId::from_uri("spotify:album:0sNOF9WDwhWunNAHPD3Baj").unwrap(); - creds_client().await.album(&birdy_uri).await.unwrap(); + creds_client().await.album(birdy_uri).await.unwrap(); } #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] async fn test_albums() { let track_uris = [ - &AlbumId::from_uri("spotify:album:41MnTivkwTO3UUJ8DrqEJJ").unwrap(), - &AlbumId::from_uri("spotify:album:6JWc4iAiJ9FjyK0B59ABb4").unwrap(), - &AlbumId::from_uri("spotify:album:6UXCm6bOO4gFlDQZV5yL37").unwrap(), + AlbumId::from_uri("spotify:album:41MnTivkwTO3UUJ8DrqEJJ").unwrap(), + AlbumId::from_uri("spotify:album:6JWc4iAiJ9FjyK0B59ABb4").unwrap(), + AlbumId::from_uri("spotify:album:6UXCm6bOO4gFlDQZV5yL37").unwrap(), ]; creds_client().await.albums(track_uris).await.unwrap(); } @@ -44,7 +44,7 @@ async fn test_album_tracks() { let birdy_uri = AlbumId::from_uri("spotify:album:6akEvsycLGftJxYudPjmqK").unwrap(); creds_client() .await - .album_track_manual(&birdy_uri, Some(2), None) + .album_track_manual(birdy_uri, Some(2), None) .await .unwrap(); } @@ -54,7 +54,7 @@ async fn test_artist_related_artists() { let birdy_uri = ArtistId::from_uri("spotify:artist:43ZHCT0cAZBISjO8DG9PnE").unwrap(); creds_client() .await - .artist_related_artists(&birdy_uri) + .artist_related_artists(birdy_uri) .await .unwrap(); } @@ -62,7 +62,7 @@ async fn test_artist_related_artists() { #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] async fn test_artist() { let birdy_uri = ArtistId::from_uri("spotify:artist:2WX2uTcsvV5OnS0inACecP").unwrap(); - creds_client().await.artist(&birdy_uri).await.unwrap(); + creds_client().await.artist(birdy_uri).await.unwrap(); } #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] @@ -71,7 +71,7 @@ async fn test_artists_albums() { creds_client() .await .artist_albums_manual( - &birdy_uri, + birdy_uri, Some(&AlbumType::Album), Some(&Market::Country(Country::UnitedStates)), Some(10), @@ -84,8 +84,8 @@ async fn test_artists_albums() { #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] async fn test_artists() { let artist_uris = [ - &ArtistId::from_uri("spotify:artist:0oSGxfWSnnOXhD2fKuz2Gy").unwrap(), - &ArtistId::from_uri("spotify:artist:3dBVyJ7JuOMt4GE9607Qin").unwrap(), + ArtistId::from_uri("spotify:artist:0oSGxfWSnnOXhD2fKuz2Gy").unwrap(), + ArtistId::from_uri("spotify:artist:3dBVyJ7JuOMt4GE9607Qin").unwrap(), ]; creds_client().await.artists(artist_uris).await.unwrap(); } @@ -95,7 +95,7 @@ async fn test_artist_top_tracks() { let birdy_uri = ArtistId::from_uri("spotify:artist:2WX2uTcsvV5OnS0inACecP").unwrap(); creds_client() .await - .artist_top_tracks(&birdy_uri, &Market::Country(Country::UnitedStates)) + .artist_top_tracks(birdy_uri, &Market::Country(Country::UnitedStates)) .await .unwrap(); } @@ -103,13 +103,13 @@ async fn test_artist_top_tracks() { #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] async fn test_audio_analysis() { let track = TrackId::from_id("06AKEBrKUckW0KREUWRnvT").unwrap(); - creds_client().await.track_analysis(&track).await.unwrap(); + creds_client().await.track_analysis(track).await.unwrap(); } #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] async fn test_audio_features() { let track = TrackId::from_uri("spotify:track:06AKEBrKUckW0KREUWRnvT").unwrap(); - creds_client().await.track_features(&track).await.unwrap(); + creds_client().await.track_features(track).await.unwrap(); } #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] @@ -121,7 +121,7 @@ async fn test_audios_features() { tracks_ids.push(track_id2); creds_client() .await - .tracks_features(&tracks_ids) + .tracks_features(tracks_ids) .await .unwrap(); } @@ -129,20 +129,20 @@ async fn test_audios_features() { #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] async fn test_user() { let birdy_uri = UserId::from_id("tuggareutangranser").unwrap(); - creds_client().await.user(&birdy_uri).await.unwrap(); + creds_client().await.user(birdy_uri).await.unwrap(); } #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] async fn test_track() { let birdy_uri = TrackId::from_uri("spotify:track:6rqhFgbbKwnb9MLmUQDhG6").unwrap(); - creds_client().await.track(&birdy_uri).await.unwrap(); + creds_client().await.track(birdy_uri).await.unwrap(); } #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] async fn test_tracks() { let track_uris = [ - &TrackId::from_uri("spotify:track:3n3Ppam7vgaVa1iaRUc9Lp").unwrap(), - &TrackId::from_uri("spotify:track:3twNvmDtFQtAd5gMKedhLD").unwrap(), + TrackId::from_uri("spotify:track:3n3Ppam7vgaVa1iaRUc9Lp").unwrap(), + TrackId::from_uri("spotify:track:3twNvmDtFQtAd5gMKedhLD").unwrap(), ]; creds_client().await.tracks(track_uris, None).await.unwrap(); } @@ -152,7 +152,7 @@ async fn test_existing_playlist() { let playlist_id = PlaylistId::from_id("37i9dQZF1DZ06evO45P0Eo").unwrap(); creds_client() .await - .playlist(&playlist_id, None, None) + .playlist(playlist_id, None, None) .await .unwrap(); } @@ -162,7 +162,7 @@ async fn test_fake_playlist() { let playlist_id = PlaylistId::from_id("fakeid").unwrap(); let playlist = creds_client() .await - .playlist(&playlist_id, None, None) + .playlist(playlist_id, None, None) .await; assert!(playlist.is_err()); } @@ -211,7 +211,7 @@ mod test_pagination { let album = AlbumId::from_uri(ALBUM).unwrap(); let names = client - .album_track(&album) + .album_track(album) .map(|track| track.unwrap().name) .collect::>() .await; diff --git a/tests/test_with_oauth.rs b/tests/test_with_oauth.rs index 08e2e78d..50ee7e45 100644 --- a/tests/test_with_oauth.rs +++ b/tests/test_with_oauth.rs @@ -195,15 +195,15 @@ async fn test_current_user_recently_played() { #[ignore] async fn test_current_user_saved_albums() { let album_ids = [ - &AlbumId::from_id("6akEvsycLGftJxYudPjmqK").unwrap(), - &AlbumId::from_id("628oezqK2qfmCjC6eXNors").unwrap(), + AlbumId::from_id("6akEvsycLGftJxYudPjmqK").unwrap(), + AlbumId::from_id("628oezqK2qfmCjC6eXNors").unwrap(), ]; let client = oauth_client().await; // First adding the albums client - .current_user_saved_albums_add(album_ids) + .current_user_saved_albums_add(album_ids.iter().map(AlbumId::as_borrowed)) .await .unwrap(); @@ -233,16 +233,16 @@ async fn test_current_user_saved_albums() { async fn test_current_user_saved_tracks_add() { let client = oauth_client().await; let tracks_ids = [ - &TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap(), - &TrackId::from_uri("spotify:track:1301WleyT98MSxVHPZCA6M").unwrap(), + TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap(), + TrackId::from_uri("spotify:track:1301WleyT98MSxVHPZCA6M").unwrap(), ]; client - .current_user_saved_tracks_add(tracks_ids) + .current_user_saved_tracks_add(tracks_ids.iter().map(TrackId::as_borrowed)) .await .unwrap(); let contains = client - .current_user_saved_tracks_contains(tracks_ids) + .current_user_saved_tracks_contains(tracks_ids.iter().map(TrackId::as_borrowed)) .await .unwrap(); // Every track should be saved @@ -324,9 +324,9 @@ async fn test_new_releases_with_from_token() { async fn test_playback() { let client = oauth_client().await; let uris = [ - Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), - Playable::Track(TrackId::from_uri("spotify:track:2DzSjFQKetFhkFCuDWhioi").unwrap()), - Playable::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), + PlayableId::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), + PlayableId::Track(TrackId::from_uri("spotify:track:2DzSjFQKetFhkFCuDWhioi").unwrap()), + PlayableId::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), ]; let devices = client.device().await.unwrap(); @@ -347,7 +347,7 @@ async fn test_playback() { // Starting playback of some songs client .start_uris_playback( - uris.to_vec(), + uris.iter().map(PlayableId::as_borrowed), Some(device_id), Some(Offset::for_position(0)), None, @@ -405,8 +405,8 @@ async fn test_playback() { #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] #[ignore] async fn test_recommendations() { - let seed_artists = [&ArtistId::from_id("4NHQUGzhtTLFvgF5SZesLK").unwrap()]; - let seed_tracks = [&TrackId::from_id("0c6xIDDpzE81m2q797ordA").unwrap()]; + let seed_artists = [ArtistId::from_id("4NHQUGzhtTLFvgF5SZesLK").unwrap()]; + let seed_tracks = [TrackId::from_id("0c6xIDDpzE81m2q797ordA").unwrap()]; let attributes = [ RecommendationsAttribute::MinEnergy(0.4), RecommendationsAttribute::MinPopularity(50), @@ -561,11 +561,11 @@ async fn test_shuffle() { async fn test_user_follow_artist() { let client = oauth_client().await; let artists = [ - &ArtistId::from_id("74ASZWbe4lXaubB36ztrGX").unwrap(), - &ArtistId::from_id("08td7MxkoHQkXnWAYD8d6Q").unwrap(), + ArtistId::from_id("74ASZWbe4lXaubB36ztrGX").unwrap(), + ArtistId::from_id("08td7MxkoHQkXnWAYD8d6Q").unwrap(), ]; - client.user_follow_artists(artists).await.unwrap(); + client.user_follow_artists(artists.iter().map(ArtistId::as_borrowed)).await.unwrap(); client.user_unfollow_artists(artists).await.unwrap(); } @@ -574,11 +574,11 @@ async fn test_user_follow_artist() { async fn test_user_follow_users() { let client = oauth_client().await; let users = [ - &UserId::from_id("exampleuser01").unwrap(), - &UserId::from_id("john").unwrap(), + UserId::from_id("exampleuser01").unwrap(), + UserId::from_id("john").unwrap(), ]; - client.user_follow_users(users).await.unwrap(); + client.user_follow_users(users.iter().map(UserId::as_borrowed)).await.unwrap(); client.user_unfollow_users(users).await.unwrap(); } @@ -589,11 +589,11 @@ async fn test_user_follow_playlist() { let playlist_id = PlaylistId::from_id("2v3iNvBX8Ay1Gt2uXtUKUT").unwrap(); client - .playlist_follow(&playlist_id, Some(true)) + .playlist_follow(playlist_id.as_borrowed(), Some(true)) .await .unwrap(); - client.playlist_unfollow(&playlist_id).await.unwrap(); + client.playlist_unfollow(playlist_id).await.unwrap(); } #[maybe_async] @@ -603,17 +603,17 @@ async fn check_playlist_create(client: &AuthCodeSpotify) -> FullPlaylist { // First creating the base playlist over which the tests will be ran let playlist = client - .user_playlist_create(&user.id, name, Some(false), None, None) + .user_playlist_create(user.id.as_borrowed(), name, Some(false), None, None) .await .unwrap(); // Making sure that the playlist has been added to the user's profile let fetched_playlist = client - .user_playlist(&user.id, Some(&playlist.id), None) + .user_playlist(user.id.as_borrowed(), Some(playlist.id.as_borrowed()), None) .await .unwrap(); assert_eq!(playlist.id, fetched_playlist.id); - let user_playlists = fetch_all(client.user_playlists(&user.id)).await; + let user_playlists = fetch_all(client.user_playlists(user.id)).await; let current_user_playlists = fetch_all(client.current_user_playlists()).await; assert_eq!(user_playlists.len(), current_user_playlists.len()); @@ -622,7 +622,7 @@ async fn check_playlist_create(client: &AuthCodeSpotify) -> FullPlaylist { let description = "A random description"; client .playlist_change_detail( - &playlist.id, + playlist.id.as_borrowed(), Some(name), Some(true), Some(description), @@ -635,8 +635,8 @@ async fn check_playlist_create(client: &AuthCodeSpotify) -> FullPlaylist { } #[maybe_async] -async fn check_num_tracks(client: &AuthCodeSpotify, playlist_id: &PlaylistId, num: i32) { - let fetched_tracks = fetch_all(client.playlist_items(playlist_id, None, None)).await; +async fn check_num_tracks(client: &AuthCodeSpotify, playlist_id: PlaylistId<'_>, num: i32) { + let fetched_tracks = fetch_all(client.playlist_items(&playlist_id.as_borrowed(), None, None)).await; assert_eq!(fetched_tracks.len() as i32, num); } @@ -644,91 +644,91 @@ async fn check_num_tracks(client: &AuthCodeSpotify, playlist_id: &PlaylistId, nu async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist) { // The tracks in the playlist, some of them repeated let tracks = [ - Playable::Track(TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap()), - Playable::Track(TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap()), - Playable::Episode(EpisodeId::from_uri("spotify/episode/381XrGKkcdNkLwfsQ4Mh5y").unwrap()), - Playable::Episode(EpisodeId::from_uri("spotify/episode/6O63eWrfWPvN41CsSyDXve").unwrap()), + PlayableId::Track(TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap()), + PlayableId::Track(TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap()), + PlayableId::Episode(EpisodeId::from_uri("spotify/episode/381XrGKkcdNkLwfsQ4Mh5y").unwrap()), + PlayableId::Episode(EpisodeId::from_uri("spotify/episode/6O63eWrfWPvN41CsSyDXve").unwrap()), ]; // Firstly adding some tracks client - .playlist_add_items(&playlist.id, tracks.to_vec(), None) + .playlist_add_items(playlist.id.as_borrowed(), tracks.iter().map(PlayableId::as_borrowed), None) .await .unwrap(); - check_num_tracks(client, &playlist.id, tracks.len() as i32).await; + check_num_tracks(client, playlist.id.as_borrowed(), tracks.len() as i32).await; // Reordering some tracks client - .playlist_reorder_items(&playlist.id, Some(0), Some(3), Some(2), None) + .playlist_reorder_items(playlist.id.as_borrowed(), Some(0), Some(3), Some(2), None) .await .unwrap(); // Making sure the number of tracks is the same - check_num_tracks(client, &playlist.id, tracks.len() as i32).await; + check_num_tracks(client, playlist.id.as_borrowed(), tracks.len() as i32).await; // Replacing the tracks let replaced_tracks = [ - Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), - Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), - Playable::Track(TrackId::from_uri("spotify:track:1301WleyT98MSxVHPZCA6M").unwrap()), - Playable::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), - Playable::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), - Playable::Episode(EpisodeId::from_id("4zugY5eJisugQj9rj8TYuh").unwrap()), - Playable::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), + PlayableId::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), + PlayableId::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), + PlayableId::Track(TrackId::from_uri("spotify:track:1301WleyT98MSxVHPZCA6M").unwrap()), + PlayableId::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), + PlayableId::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), + PlayableId::Episode(EpisodeId::from_id("4zugY5eJisugQj9rj8TYuh").unwrap()), + PlayableId::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), ]; client - .playlist_replace_items(&playlist.id, replaced_tracks.to_vec()) + .playlist_replace_items(playlist.id.as_borrowed(), replaced_tracks.iter().map(|t| t.as_borrowed())) .await .unwrap(); // Making sure the number of tracks is updated - check_num_tracks(client, &playlist.id, replaced_tracks.len() as i32).await; + check_num_tracks(client, playlist.id.as_borrowed(), replaced_tracks.len() as i32).await; // Removes a few specific tracks let tracks = [ ItemPositions { - id: Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), + id: PlayableId::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), positions: &[0], }, ItemPositions { - id: Playable::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), + id: PlayableId::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), positions: &[4, 6], }, ]; client - .playlist_remove_specific_occurrences_of_items(&playlist.id, tracks, None) + .playlist_remove_specific_occurrences_of_items(playlist.id.as_borrowed(), tracks, None) .await .unwrap(); // Making sure three tracks were removed - check_num_tracks(client, &playlist.id, replaced_tracks.len() as i32 - 3).await; + check_num_tracks(client, playlist.id.as_borrowed(), replaced_tracks.len() as i32 - 3).await; // Removes all occurrences of two tracks let to_remove = vec![ - Playable::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), - Playable::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), + PlayableId::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), + PlayableId::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), ]; client - .playlist_remove_all_occurrences_of_items(&playlist.id, to_remove, None) + .playlist_remove_all_occurrences_of_items(playlist.id.as_borrowed(), to_remove, None) .await .unwrap(); // Making sure two more tracks were removed - check_num_tracks(client, &playlist.id, replaced_tracks.len() as i32 - 5).await; + check_num_tracks(client, playlist.id.as_borrowed(), replaced_tracks.len() as i32 - 5).await; } #[maybe_async] async fn check_playlist_follow(client: &AuthCodeSpotify, playlist: &FullPlaylist) { let user_ids = [ - &UserId::from_id("possan").unwrap(), - &UserId::from_id("elogain").unwrap(), + UserId::from_id("possan").unwrap(), + UserId::from_id("elogain").unwrap(), ]; // It's a new playlist, so it shouldn't have any followers let following = client - .playlist_check_follow(&playlist.id, &user_ids) + .playlist_check_follow(playlist.id.as_borrowed(), &user_ids) .await .unwrap(); assert_eq!(following, vec![false, false]); // Finally unfollowing the playlist in order to clean it up - client.playlist_unfollow(&playlist.id).await.unwrap(); + client.playlist_unfollow(playlist.id.as_borrowed()).await.unwrap(); } #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] @@ -769,10 +769,10 @@ async fn test_add_queue() { // NOTE: unfortunately it's impossible to revert this test let birdy_uri = - Playable::Track(TrackId::from_uri("spotify:track:6rqhFgbbKwnb9MLmUQDhG6").unwrap()); + PlayableId::Track(TrackId::from_uri("spotify:track:6rqhFgbbKwnb9MLmUQDhG6").unwrap()); oauth_client() .await - .add_item_to_queue(&birdy_uri, None) + .add_item_to_queue(birdy_uri, None) .await .unwrap(); } @@ -781,8 +781,8 @@ async fn test_add_queue() { #[ignore] async fn test_get_several_shows() { let shows = [ - &ShowId::from_id("5CfCWKI5pZ28U0uOzXkDHe").unwrap(), - &ShowId::from_id("5as3aKmN2k11yfDDDSrvaZ").unwrap(), + ShowId::from_id("5CfCWKI5pZ28U0uOzXkDHe").unwrap(), + ShowId::from_id("5as3aKmN2k11yfDDDSrvaZ").unwrap(), ]; oauth_client() @@ -796,8 +796,8 @@ async fn test_get_several_shows() { #[ignore] async fn test_get_several_episodes() { let episodes = [ - &EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap(), - &EpisodeId::from_id("4zugY5eJisugQj9rj8TYuh").unwrap(), + EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap(), + EpisodeId::from_id("4zugY5eJisugQj9rj8TYuh").unwrap(), ]; oauth_client() .await From b25790aedc544025f3f3b3733602fd72b4dc05be Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Thu, 2 Jun 2022 17:24:47 +0200 Subject: [PATCH 08/28] Rename as_borrowed to as_ref --- rspotify-model/src/idtypes.rs | 6 ++--- rspotify-model/src/lib.rs | 4 +-- src/clients/base.rs | 2 +- tests/test_with_oauth.rs | 46 +++++++++++++++++------------------ 4 files changed, 29 insertions(+), 29 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 1626d8d0..d4913420 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -402,7 +402,7 @@ macro_rules! define_idtypes { } impl<'a> $name<'a> { - pub fn as_borrowed(&'a self) -> $name<'a> { + pub fn as_ref(&'a self) -> $name<'a> { Self(std::borrow::Cow::Borrowed(self.0.as_ref())) } } @@ -490,10 +490,10 @@ macro_rules! define_idgroups { } } - pub fn as_borrowed(&'a self) -> Self { + pub fn as_ref(&'a self) -> Self { match self { $( - $name::$variant_name(x) => $name::$variant_name(x.as_borrowed()), + $name::$variant_name(x) => $name::$variant_name(x.as_ref()), )+ } } diff --git a/rspotify-model/src/lib.rs b/rspotify-model/src/lib.rs index d1b18134..6c80573b 100644 --- a/rspotify-model/src/lib.rs +++ b/rspotify-model/src/lib.rs @@ -55,8 +55,8 @@ impl PlayableItem { /// which case this function will return `None`. pub fn id(&self) -> Option> { match self { - PlayableItem::Track(t) => t.id.as_ref().map(|t| PlayableId::Track(t.as_borrowed())), - PlayableItem::Episode(e) => Some(PlayableId::Episode(e.id.as_borrowed())), + PlayableItem::Track(t) => t.id.as_ref().map(|t| PlayableId::Track(t.as_ref())), + PlayableItem::Episode(e) => Some(PlayableId::Episode(e.id.as_ref())), } } } diff --git a/src/clients/base.rs b/src/clients/base.rs index cff966a9..f7a9fcea 100644 --- a/src/clients/base.rs +++ b/src/clients/base.rs @@ -1033,7 +1033,7 @@ where paginate( move |limit, offset| { self.playlist_items_manual( - playlist_id.as_borrowed(), + playlist_id.as_ref(), fields, market, Some(limit), diff --git a/tests/test_with_oauth.rs b/tests/test_with_oauth.rs index 50ee7e45..c083db06 100644 --- a/tests/test_with_oauth.rs +++ b/tests/test_with_oauth.rs @@ -203,7 +203,7 @@ async fn test_current_user_saved_albums() { // First adding the albums client - .current_user_saved_albums_add(album_ids.iter().map(AlbumId::as_borrowed)) + .current_user_saved_albums_add(album_ids.iter().map(AlbumId::as_ref)) .await .unwrap(); @@ -237,12 +237,12 @@ async fn test_current_user_saved_tracks_add() { TrackId::from_uri("spotify:track:1301WleyT98MSxVHPZCA6M").unwrap(), ]; client - .current_user_saved_tracks_add(tracks_ids.iter().map(TrackId::as_borrowed)) + .current_user_saved_tracks_add(tracks_ids.iter().map(TrackId::as_ref)) .await .unwrap(); let contains = client - .current_user_saved_tracks_contains(tracks_ids.iter().map(TrackId::as_borrowed)) + .current_user_saved_tracks_contains(tracks_ids.iter().map(TrackId::as_ref)) .await .unwrap(); // Every track should be saved @@ -347,7 +347,7 @@ async fn test_playback() { // Starting playback of some songs client .start_uris_playback( - uris.iter().map(PlayableId::as_borrowed), + uris.iter().map(PlayableId::as_ref), Some(device_id), Some(Offset::for_position(0)), None, @@ -565,7 +565,7 @@ async fn test_user_follow_artist() { ArtistId::from_id("08td7MxkoHQkXnWAYD8d6Q").unwrap(), ]; - client.user_follow_artists(artists.iter().map(ArtistId::as_borrowed)).await.unwrap(); + client.user_follow_artists(artists.iter().map(ArtistId::as_ref)).await.unwrap(); client.user_unfollow_artists(artists).await.unwrap(); } @@ -578,7 +578,7 @@ async fn test_user_follow_users() { UserId::from_id("john").unwrap(), ]; - client.user_follow_users(users.iter().map(UserId::as_borrowed)).await.unwrap(); + client.user_follow_users(users.iter().map(UserId::as_ref)).await.unwrap(); client.user_unfollow_users(users).await.unwrap(); } @@ -589,7 +589,7 @@ async fn test_user_follow_playlist() { let playlist_id = PlaylistId::from_id("2v3iNvBX8Ay1Gt2uXtUKUT").unwrap(); client - .playlist_follow(playlist_id.as_borrowed(), Some(true)) + .playlist_follow(playlist_id.as_ref(), Some(true)) .await .unwrap(); @@ -603,13 +603,13 @@ async fn check_playlist_create(client: &AuthCodeSpotify) -> FullPlaylist { // First creating the base playlist over which the tests will be ran let playlist = client - .user_playlist_create(user.id.as_borrowed(), name, Some(false), None, None) + .user_playlist_create(user.id.as_ref(), name, Some(false), None, None) .await .unwrap(); // Making sure that the playlist has been added to the user's profile let fetched_playlist = client - .user_playlist(user.id.as_borrowed(), Some(playlist.id.as_borrowed()), None) + .user_playlist(user.id.as_ref(), Some(playlist.id.as_ref()), None) .await .unwrap(); assert_eq!(playlist.id, fetched_playlist.id); @@ -622,7 +622,7 @@ async fn check_playlist_create(client: &AuthCodeSpotify) -> FullPlaylist { let description = "A random description"; client .playlist_change_detail( - playlist.id.as_borrowed(), + playlist.id.as_ref(), Some(name), Some(true), Some(description), @@ -636,7 +636,7 @@ async fn check_playlist_create(client: &AuthCodeSpotify) -> FullPlaylist { #[maybe_async] async fn check_num_tracks(client: &AuthCodeSpotify, playlist_id: PlaylistId<'_>, num: i32) { - let fetched_tracks = fetch_all(client.playlist_items(&playlist_id.as_borrowed(), None, None)).await; + let fetched_tracks = fetch_all(client.playlist_items(&playlist_id.as_ref(), None, None)).await; assert_eq!(fetched_tracks.len() as i32, num); } @@ -652,18 +652,18 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist // Firstly adding some tracks client - .playlist_add_items(playlist.id.as_borrowed(), tracks.iter().map(PlayableId::as_borrowed), None) + .playlist_add_items(playlist.id.as_ref(), tracks.iter().map(PlayableId::as_ref), None) .await .unwrap(); - check_num_tracks(client, playlist.id.as_borrowed(), tracks.len() as i32).await; + check_num_tracks(client, playlist.id.as_ref(), tracks.len() as i32).await; // Reordering some tracks client - .playlist_reorder_items(playlist.id.as_borrowed(), Some(0), Some(3), Some(2), None) + .playlist_reorder_items(playlist.id.as_ref(), Some(0), Some(3), Some(2), None) .await .unwrap(); // Making sure the number of tracks is the same - check_num_tracks(client, playlist.id.as_borrowed(), tracks.len() as i32).await; + check_num_tracks(client, playlist.id.as_ref(), tracks.len() as i32).await; // Replacing the tracks let replaced_tracks = [ @@ -676,11 +676,11 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist PlayableId::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), ]; client - .playlist_replace_items(playlist.id.as_borrowed(), replaced_tracks.iter().map(|t| t.as_borrowed())) + .playlist_replace_items(playlist.id.as_ref(), replaced_tracks.iter().map(|t| t.as_ref())) .await .unwrap(); // Making sure the number of tracks is updated - check_num_tracks(client, playlist.id.as_borrowed(), replaced_tracks.len() as i32).await; + check_num_tracks(client, playlist.id.as_ref(), replaced_tracks.len() as i32).await; // Removes a few specific tracks let tracks = [ @@ -694,11 +694,11 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist }, ]; client - .playlist_remove_specific_occurrences_of_items(playlist.id.as_borrowed(), tracks, None) + .playlist_remove_specific_occurrences_of_items(playlist.id.as_ref(), tracks, None) .await .unwrap(); // Making sure three tracks were removed - check_num_tracks(client, playlist.id.as_borrowed(), replaced_tracks.len() as i32 - 3).await; + check_num_tracks(client, playlist.id.as_ref(), replaced_tracks.len() as i32 - 3).await; // Removes all occurrences of two tracks let to_remove = vec![ @@ -706,11 +706,11 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist PlayableId::Episode(EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap()), ]; client - .playlist_remove_all_occurrences_of_items(playlist.id.as_borrowed(), to_remove, None) + .playlist_remove_all_occurrences_of_items(playlist.id.as_ref(), to_remove, None) .await .unwrap(); // Making sure two more tracks were removed - check_num_tracks(client, playlist.id.as_borrowed(), replaced_tracks.len() as i32 - 5).await; + check_num_tracks(client, playlist.id.as_ref(), replaced_tracks.len() as i32 - 5).await; } #[maybe_async] @@ -722,13 +722,13 @@ async fn check_playlist_follow(client: &AuthCodeSpotify, playlist: &FullPlaylist // It's a new playlist, so it shouldn't have any followers let following = client - .playlist_check_follow(playlist.id.as_borrowed(), &user_ids) + .playlist_check_follow(playlist.id.as_ref(), &user_ids) .await .unwrap(); assert_eq!(following, vec![false, false]); // Finally unfollowing the playlist in order to clean it up - client.playlist_unfollow(playlist.id.as_borrowed()).await.unwrap(); + client.playlist_unfollow(playlist.id.as_ref()).await.unwrap(); } #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] From 17996150a978021914abf3c02af9427b9cd561d7 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Thu, 2 Jun 2022 17:43:00 +0200 Subject: [PATCH 09/28] Fix part of the docs --- rspotify-model/src/idtypes.rs | 51 ++++++++++++++++------------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index d4913420..cc1d00ab 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -15,6 +15,11 @@ //! * [`Type::Show`] => [`ShowId`] //! * [`Type::Episode`] => [`EpisodeId`] //! +//! These types are just wrappers for [`Cow`], so their usage should be +//! quite similar overall. +//! +//! [`Cow`]: [`std::borrow::Cow`] +//! //! ## Examples //! //! If an endpoint requires a `TrackId`, you may pass it as: @@ -22,10 +27,10 @@ //! ``` //! use rspotify_model::{Id, TrackId}; //! -//! fn pause_track(id: &TrackId) { /* ... */ } +//! fn pause_track(id: TrackId<'_>) { /* ... */ } //! //! let id = TrackId::from_id("4iV5W9uYEdYUVa79Axb7Rh").unwrap(); -//! pause_track(&id); +//! pause_track(id); //! ``` //! //! Notice how this way it's type safe; the following example would fail at @@ -34,10 +39,10 @@ //! ```compile_fail //! use rspotify_model::{Id, TrackId, EpisodeId}; //! -//! fn pause_track(id: &TrackId) { /* ... */ } +//! fn pause_track(id: TrackId<'_>) { /* ... */ } //! //! let id = EpisodeId::from_id("4iV5W9uYEdYUVa79Axb7Rh").unwrap(); -//! pause_track(&id); +//! pause_track(id); //! ``` //! //! And this would panic because it's a `TrackId` but its URI string specifies @@ -46,10 +51,10 @@ //! ```should_panic //! use rspotify_model::{Id, TrackId}; //! -//! fn pause_track(id: &TrackId) { /* ... */ } +//! fn pause_track(id: TrackId<'_>) { /* ... */ } //! //! let id = TrackId::from_uri("spotify:album:6akEvsycLGftJxYudPjmqK").unwrap(); -//! pause_track(&id); +//! pause_track(id); //! ``` //! //! A more complex example where an endpoint takes a vector of IDs of different @@ -58,9 +63,9 @@ //! ``` //! use rspotify_model::{Id, TrackId, EpisodeId, PlayableId}; //! -//! fn track(id: &TrackId) { /* ... */ } -//! fn episode(id: &EpisodeId) { /* ... */ } -//! fn add_to_queue(id: &[&dyn PlayableId]) { /* ... */ } +//! fn track(id: TrackId<'_>) { /* ... */ } +//! fn episode(id: EpisodeId<'_>) { /* ... */ } +//! fn add_to_queue(id: &[PlayableId<'_>]) { /* ... */ } //! //! let tracks = &[ //! TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap(), @@ -72,20 +77,20 @@ //! ]; //! //! // First we get some info about the tracks and episodes -//! let track_info = tracks.iter().map(|id| track(id)).collect::>(); -//! let ep_info = episodes.iter().map(|id| episode(id)).collect::>(); +//! let track_info = tracks.iter().map(|id| track(id.as_ref())).collect::>(); +//! let ep_info = episodes.iter().map(|id| episode(id.as_ref())).collect::>(); //! println!("Track info: {:?}", track_info); //! println!("Episode info: {:?}", ep_info); //! //! // And then we add both the tracks and episodes to the queue //! let playable = tracks //! .iter() -//! .map(|id| id as &dyn PlayableId) +//! .map(PlayableId) //! .chain( -//! episodes.iter().map(|id| id as &dyn PlayableId) +//! episodes.iter().map(PlayableId) //! ) -//! .collect::>(); -//! add_to_queue(&playable); +//! .collect::>(); +//! add_to_queue(playable); //! ``` use serde::{Deserialize, Serialize}; @@ -114,20 +119,10 @@ pub enum IdError { InvalidId, } -/// The main interface for an ID. -/// -/// # Implementation note -/// -/// Note that for IDs to be useful their trait must be object-safe. Otherwise, -/// it wouldn't be possible to use `Vec>` to take multiple kinds of -/// IDs or just `Box` in case the type wasn't known at compile time. -/// This is why this trait includes both [`Self::_type`] and -/// [`Self::_type_static`], and why all of the static methods use `where Self: -/// Sized`. +/// The main interface for an ID. See the [`module level documentation`] for +/// more information. /// -/// Unfortunately, since `where Self: Sized` has to be enforced, IDs cannot be -/// simply a `str` internally because these aren't sized. Thus, IDs will have the -/// slight overhead of having to use an owned type like `String`. +/// [`module level documentation`]: [`crate::idtypes`] pub trait Id<'a> { /// The type of the ID. const TYPE: Type; From 3f787bb663c7810288370a2e3929c0938023c0c0 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Thu, 2 Jun 2022 18:35:56 +0200 Subject: [PATCH 10/28] Considerably neater implementation --- rspotify-model/src/idtypes.rs | 388 ++++++++++++++++------------------ 1 file changed, 185 insertions(+), 203 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index cc1d00ab..7ce143cc 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -93,6 +93,7 @@ //! add_to_queue(playable); //! ``` +use enum_dispatch::enum_dispatch; use serde::{Deserialize, Serialize}; use strum::Display; use thiserror::Error; @@ -123,143 +124,59 @@ pub enum IdError { /// more information. /// /// [`module level documentation`]: [`crate::idtypes`] +#[enum_dispatch] pub trait Id<'a> { - /// The type of the ID. - const TYPE: Type; - - /// Spotify object Id (guaranteed to be valid for that type) + /// Returns the inner Spotify object Id, which is guaranteed to be valid for + /// its type. fn id(&self) -> &str; - /// Only returns `true` in case the given string is valid according to that - /// specific Id (e.g., some may require alphanumeric characters only). - fn id_is_valid(id: &str) -> bool; - - /// Initialize the Id without checking its validity. - /// - /// # Safety - /// - /// The string passed to this method must be made out of valid characters - /// only; otherwise undefined behaviour may occur. - unsafe fn from_id_unchecked(id: &'a str) -> Self - where - Self: Sized; + /// The type of the ID, as a function. + fn _type(&self) -> Type; - /// Spotify object URI in a well-known format: `spotify:type:id` + /// Returns a Spotify object URI in a well-known format: `spotify:type:id`. /// /// Examples: `spotify:album:6IcGNaXFRf5Y1jc7QsE9O2`, /// `spotify:track:4y4VO05kYgUTo2bzbox1an`. fn uri(&self) -> String { - format!("spotify:{}:{}", Self::TYPE, self.id()) + format!("spotify:{}:{}", self._type(), self.id()) } - /// Full Spotify object URL, can be opened in a browser + /// Returns a full Spotify object URL that can be opened in a browser. /// /// Examples: `https://open.spotify.com/track/4y4VO05kYgUTo2bzbox1an`, /// `https://open.spotify.com/artist/2QI8e2Vwgg9KXOz2zjcrkI`. fn url(&self) -> String { - format!("https://open.spotify.com/{}/{}", Self::TYPE, self.id()) - } - - /// Parse Spotify Id from string slice. - /// - /// A valid Spotify object id must be a non-empty string with valid - /// characters. - /// - /// # Errors: - /// - /// - `IdError::InvalidId` - if `id` contains invalid characters. - fn from_id(id: &'a str) -> Result - where - Self: Sized, - { - if Self::id_is_valid(id) { - // Safe, we've just checked that the Id is valid. - Ok(unsafe { Self::from_id_unchecked(id) }) - } else { - Err(IdError::InvalidId) - } - } - - /// Parse Spotify URI from string slice - /// - /// Spotify URI must be in one of the following formats: - /// `spotify:{type}:{id}` or `spotify/{type}/{id}`. - /// Where `{type}` is one of `artist`, `album`, `track`, `playlist`, `user`, - /// `show`, or `episode`, and `{id}` is a non-empty valid string. - /// - /// Examples: `spotify:album:6IcGNaXFRf5Y1jc7QsE9O2`, - /// `spotify/track/4y4VO05kYgUTo2bzbox1an`. - /// - /// # Errors: - /// - /// - `IdError::InvalidPrefix` - if `uri` is not started with `spotify:` - /// or `spotify/`, - /// - `IdError::InvalidType` - if type part of an `uri` is not a valid - /// Spotify type `T`, - /// - `IdError::InvalidId` - if id part of an `uri` is not a valid id, - /// - `IdError::InvalidFormat` - if it can't be splitted into type and - /// id parts. - fn from_uri(uri: &'a str) -> Result - where - Self: Sized, - { - let mut chars = uri - .strip_prefix("spotify") - .ok_or(IdError::InvalidPrefix)? - .chars(); - let sep = match chars.next() { - Some(ch) if ch == '/' || ch == ':' => ch, - _ => return Err(IdError::InvalidPrefix), - }; - let rest = chars.as_str(); - - let (tpe, id) = rest - .rfind(sep) - .map(|mid| rest.split_at(mid)) - .ok_or(IdError::InvalidFormat)?; - - // Note that in case the type isn't known at compile time, any type will - // be accepted. - match tpe.parse::() { - Ok(tpe) if tpe == Self::TYPE => Self::from_id(&id[1..]), - _ => Err(IdError::InvalidType), - } + format!("https://open.spotify.com/{}/{}", self._type(), self.id()) } +} - /// Parse Spotify id or URI from string slice - /// - /// Spotify URI must be in one of the following formats: - /// `spotify:{type}:{id}` or `spotify/{type}/{id}`. - /// Where `{type}` is one of `artist`, `album`, `track`, `playlist`, `user`, - /// `show`, or `episode`, and `{id}` is a non-empty valid string. The URI - /// must be match with the ID's type (`Id::TYPE`), otherwise - /// `IdError::InvalidType` error is returned. - /// - /// Examples: `spotify:album:6IcGNaXFRf5Y1jc7QsE9O2`, - /// `spotify/track/4y4VO05kYgUTo2bzbox1an`. - /// - /// If input string is not a valid Spotify URI (it's not started with - /// `spotify:` or `spotify/`), it must be a valid Spotify object ID, - /// i.e. a non-empty valid string. - /// - /// # Errors: - /// - /// - `IdError::InvalidType` - if `id_or_uri` is an URI, and it's type part - /// is not equal to `T`, - /// - `IdError::InvalidId` - either if `id_or_uri` is an URI with invalid id - /// part, or it's an invalid id (id is invalid if it contains valid - /// characters), - /// - `IdError::InvalidFormat` - if `id_or_uri` is an URI, and it can't be - /// split into type and id parts. - fn from_id_or_uri(id_or_uri: &'a str) -> Result - where - Self: Sized, - { - match Self::from_uri(id_or_uri) { - Ok(id) => Ok(id), - Err(IdError::InvalidPrefix) => Self::from_id(id_or_uri), - Err(error) => Err(error), - } +/// A lower level function to parse a URI into both its type and its actual Id. +/// Note that this function doesn't check the validity of the returned Id, e.g., +/// if that it's alphanumeric; that should be done in `Id::from_id`. +/// +/// This is only useful for advanced cases, such as implementing your own Id +/// type. +pub fn parse_uri(uri: &str) -> Result<(Type, &str), IdError> { + let mut chars = uri + .strip_prefix("spotify") + .ok_or(IdError::InvalidPrefix)? + .chars(); + let sep = match chars.next() { + Some(ch) if ch == '/' || ch == ':' => ch, + _ => return Err(IdError::InvalidPrefix), + }; + let rest = chars.as_str(); + + let (tpe, id) = rest + .rfind(sep) + .map(|mid| rest.split_at(mid)) + .ok_or(IdError::InvalidFormat)?; + + // Note that in case the type isn't known at compile time, + // any type will be accepted. + match tpe.parse::() { + Ok(tpe) => Ok((tpe, &id[1..])), + _ => Err(IdError::InvalidType), } } @@ -299,25 +216,130 @@ macro_rules! define_idtypes { } } - impl<'a> Id<'a> for $name<'a> { + impl<'a> $name<'a> { + /// The type of the Id, as a constant. const TYPE: Type = Type::$type; + /// Only returns `true` in case the given string is valid + /// according to that specific Id (e.g., some may require + /// alphanumeric characters only). #[inline] - fn id(&self) -> &str { - &self.0 - } - - #[inline] - fn id_is_valid(id: &str) -> bool { + pub fn id_is_valid(id: &str) -> bool { // TODO: make prettier? const VALID_FN: fn(&str) -> bool = $validity; VALID_FN(id) } + /// Initialize the Id without checking its validity. + /// + /// # Safety + /// + /// The string passed to this method must be made out of valid + /// characters only; otherwise undefined behaviour may occur. #[inline] - unsafe fn from_id_unchecked(id: &'a str) -> Self { + pub unsafe fn from_id_unchecked(id: &'a str) -> Self { Self(std::borrow::Cow::Borrowed(id)) } + + /// Parse Spotify Id from string slice. + /// + /// A valid Spotify object id must be a non-empty string with + /// valid characters. + /// + /// # Errors: + /// + /// - `IdError::InvalidId` - if `id` contains invalid characters. + fn from_id(id: &'a str) -> Result { + if Self::id_is_valid(id) { + // Safe, we've just checked that the Id is valid. + Ok(unsafe { Self::from_id_unchecked(id) }) + } else { + Err(IdError::InvalidId) + } + } + + /// Parse Spotify URI from string slice + /// + /// Spotify URI must be in one of the following formats: + /// `spotify:{type}:{id}` or `spotify/{type}/{id}`. + /// Where `{type}` is one of `artist`, `album`, `track`, + /// `playlist`, `user`, `show`, or `episode`, and `{id}` is a + /// non-empty valid string. + /// + /// Examples: `spotify:album:6IcGNaXFRf5Y1jc7QsE9O2`, + /// `spotify/track/4y4VO05kYgUTo2bzbox1an`. + /// + /// # Errors: + /// + /// - `IdError::InvalidPrefix` - if `uri` is not started with + /// `spotify:` or `spotify/`, + /// - `IdError::InvalidType` - if type part of an `uri` is not a + /// valid Spotify type `T`, + /// - `IdError::InvalidId` - if id part of an `uri` is not a + /// valid id, + /// - `IdError::InvalidFormat` - if it can't be splitted into + /// type and id parts. + pub fn from_uri(uri: &'a str) -> Result { + let (tpe, id) = parse_uri(uri)?; + if tpe == Type::$type { + Self::from_id(id) + } else { + Err(IdError::InvalidType) + } + } + + /// Parse Spotify id or URI from string slice + /// + /// Spotify URI must be in one of the following formats: + /// `spotify:{type}:{id}` or `spotify/{type}/{id}`. + /// Where `{type}` is one of `artist`, `album`, `track`, + /// `playlist`, `user`, `show`, or `episode`, and `{id}` is a + /// non-empty valid string. The URI must be match with the ID's + /// type (`Id::TYPE`), otherwise `IdError::InvalidType` error is + /// returned. + /// + /// Examples: `spotify:album:6IcGNaXFRf5Y1jc7QsE9O2`, + /// `spotify/track/4y4VO05kYgUTo2bzbox1an`. + /// + /// If input string is not a valid Spotify URI (it's not started + /// with `spotify:` or `spotify/`), it must be a valid Spotify + /// object ID, i.e. a non-empty valid string. + /// + /// # Errors: + /// + /// - `IdError::InvalidType` - if `id_or_uri` is an URI, and + /// it's type part is not equal to `T`, + /// - `IdError::InvalidId` - either if `id_or_uri` is an URI + /// with invalid id part, or it's an invalid id (id is invalid + /// if it contains valid characters), + /// - `IdError::InvalidFormat` - if `id_or_uri` is an URI, and + /// it can't be split into type and id parts. + pub fn from_id_or_uri(id_or_uri: &'a str) -> Result { + match Self::from_uri(id_or_uri) { + Ok(id) => Ok(id), + Err(IdError::InvalidPrefix) => Self::from_id(id_or_uri), + Err(error) => Err(error), + } + } + + // TODO: better explanation? + /// Useful to keep using an Id without having to clone it. + #[inline] + pub fn as_ref(&'a self) -> $name<'a> { + Self(std::borrow::Cow::Borrowed(self.0.as_ref())) + } + } + + impl<'a> Id<'a> for $name<'a> { + #[inline] + fn id(&self) -> &str { + &self.0 + } + + #[inline] + fn _type(&self) -> Type { + Self::TYPE + } } // Deserialization may take either an Id or an URI, so its @@ -381,13 +403,6 @@ macro_rules! define_idtypes { } } - /// Cheap conversion to `str` - impl AsRef for $name<'_> { - fn as_ref(&self) -> &str { - self.id() - } - } - /// `Id`s may be borrowed as `str` the same way `Box` may be /// borrowed as `T` or `String` as `str` impl std::borrow::Borrow for $name<'_> { @@ -396,12 +411,6 @@ macro_rules! define_idtypes { } } - impl<'a> $name<'a> { - pub fn as_ref(&'a self) -> $name<'a> { - Self(std::borrow::Cow::Borrowed(self.0.as_ref())) - } - } - /// Displaying the ID shows its URI impl std::fmt::Display for $name<'_> { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { @@ -445,71 +454,44 @@ define_idtypes!( } ); -// TODO: replace with `enum_dispatch`? -macro_rules! define_idgroups { - ($(pub enum $name:ident { - $( - $variant_name:ident($variant_type:ident) - ),+ - $(,)? - }),+) => { - $( - pub enum $name<'a> { - $( - $variant_name($variant_type<'a>), - )+ - } - - // TODO: turn into `impl Id`? - // TODO: also implement `into_owned` and etc - impl<'a> $name<'a> { - pub fn uri(&self) -> String { - match self { - $( - $name::$variant_name(x) => x.uri(), - )+ - } - } - pub fn url(&self) -> String { - match self { - $( - $name::$variant_name(x) => x.url(), - )+ - } - } - pub const fn _type(&self) -> Type { - match self { - $( - $name::$variant_name(_) => $variant_type::TYPE, - )+ - } - } - - pub fn as_ref(&'a self) -> Self { - match self { - $( - $name::$variant_name(x) => $name::$variant_name(x.as_ref()), - )+ - } - } - } - )+ +// Grouping up the IDs into more specific traits with the help of +// `enum_dispatch`, which is not only easier to use than `dyn`, but also more +// efficient. +#[enum_dispatch(Id)] +pub enum PlayContextId<'a> { + Artist(ArtistId<'a>), + Album(AlbumId<'a>), + Playlist(PlaylistId<'a>), + Show(ShowId<'a>), +} +// These don't work with `enum_dispatch`, unfortunately. +impl<'a> PlayContextId<'a> { + #[inline] + pub fn as_ref(&'a self) -> PlayContextId<'a> { + match self { + PlayContextId::Artist(x) => PlayContextId::Artist(x.as_ref()), + PlayContextId::Album(x) => PlayContextId::Album(x.as_ref()), + PlayContextId::Playlist(x) => PlayContextId::Playlist(x.as_ref()), + PlayContextId::Show(x) => PlayContextId::Show(x.as_ref()), + } } } -// Grouping up the IDs into more specific traits -define_idgroups!( - pub enum PlayContextId { - Artist(ArtistId), - Album(AlbumId), - Playlist(PlaylistId), - Show(ShowId), - }, - pub enum PlayableId { - Track(TrackId), - Episode(EpisodeId), +#[enum_dispatch(Id)] +pub enum PlayableId<'a> { + Track(TrackId<'a>), + Episode(EpisodeId<'a>), +} +// These don't work with `enum_dispatch`, unfortunately. +impl<'a> PlayableId<'a> { + #[inline] + pub fn as_ref(&'a self) -> PlayableId<'a> { + match self { + PlayableId::Track(x) => PlayableId::Track(x.as_ref()), + PlayableId::Episode(x) => PlayableId::Episode(x.as_ref()), + } } -); +} #[cfg(test)] mod test { From ad36d6a178ad0451199aa1d0aa42a519b94a5892 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Thu, 2 Jun 2022 18:42:25 +0200 Subject: [PATCH 11/28] Fix tests & docs --- rspotify-model/src/idtypes.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 7ce143cc..53a430de 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -67,11 +67,11 @@ //! fn episode(id: EpisodeId<'_>) { /* ... */ } //! fn add_to_queue(id: &[PlayableId<'_>]) { /* ... */ } //! -//! let tracks = &[ +//! let tracks = [ //! TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap(), //! TrackId::from_uri("spotify:track:5iKndSu1XI74U2OZePzP8L").unwrap(), //! ]; -//! let episodes = &[ +//! let episodes = [ //! EpisodeId::from_id("0lbiy3LKzIY2fnyjioC11p").unwrap(), //! EpisodeId::from_id("4zugY5eJisugQj9rj8TYuh").unwrap(), //! ]; @@ -84,13 +84,13 @@ //! //! // And then we add both the tracks and episodes to the queue //! let playable = tracks -//! .iter() -//! .map(PlayableId) +//! .into_iter() +//! .map(|t| t.as_ref().into()) //! .chain( -//! episodes.iter().map(PlayableId) +//! episodes.into_iter().map(|e| e.as_ref().into()) //! ) //! .collect::>(); -//! add_to_queue(playable); +//! add_to_queue(&playable); //! ``` use enum_dispatch::enum_dispatch; @@ -108,15 +108,15 @@ use crate::Type; /// See also [`Id`](crate::idtypes::Id) for details. #[derive(Debug, PartialEq, Eq, Clone, Copy, Display, Error)] pub enum IdError { - /// Spotify URI prefix is not `spotify:` or `spotify/` + /// Spotify URI prefix is not `spotify:` or `spotify/`. InvalidPrefix, - /// Spotify URI can't be split into type and id parts - /// (e.g. it has invalid separator) + /// Spotify URI can't be split into type and id parts (e.g., it has invalid + /// separator). InvalidFormat, /// Spotify URI has invalid type name, or id has invalid type in a given - /// context (e.g. a method expects a track id, but artist id is provided) + /// context (e.g. a method expects a track id, but artist id is provided). InvalidType, - /// Spotify id is invalid (empty or contains invalid characters) + /// Spotify id is invalid (empty or contains invalid characters). InvalidId, } @@ -249,7 +249,7 @@ macro_rules! define_idtypes { /// # Errors: /// /// - `IdError::InvalidId` - if `id` contains invalid characters. - fn from_id(id: &'a str) -> Result { + pub fn from_id(id: &'a str) -> Result { if Self::id_is_valid(id) { // Safe, we've just checked that the Id is valid. Ok(unsafe { Self::from_id_unchecked(id) }) From a5fbd2514a204783ca2008652288d85c97c5581f Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sat, 11 Jun 2022 12:28:14 +0200 Subject: [PATCH 12/28] Back to double-collect join_ids --- src/lib.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 1f86917d..3de5c026 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -297,13 +297,8 @@ pub(in crate) fn generate_random_string(length: usize, alphabet: &[u8]) -> Strin // Hack: turning this into a macro (best if avoided). #[inline] pub(in crate) fn join_ids<'a, T: Id<'a> + 'a>(ids: impl IntoIterator) -> String { - let mut ids = ids.into_iter(); - let mut joined = if let Some(first) = ids.next() { String::from(first.id()) } else { return String::new() }; - for id in ids { - joined.push_str(","); - joined.push_str(id.id()); - } - joined + let ids = ids.into_iter().collect::>(); + ids.iter().map(Id::id).collect::>().join(",") } #[inline] From 3d68efba4871bf32da574ad8d182419950ba6c6a Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sat, 11 Jun 2022 12:47:45 +0200 Subject: [PATCH 13/28] Better docs --- rspotify-model/src/idtypes.rs | 83 +++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 38 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 53a430de..a2e3f8d2 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -1,11 +1,12 @@ -//! This module defines the necessary elements in order to represent Spotify IDs -//! and URIs with type safety and no overhead. +//! This module makes it possible to represent Spotify IDs and URIs with type +//! safety and almost no overhead. //! //! ## Concrete IDs //! //! The trait [`Id`] is the central element of this module. It's implemented by -//! different kinds of ID ([`AlbumId`], [`EpisodeId`], etc), and implements the -//! logic to initialize and use IDs. +//! all kinds of ID, and includes the main functionality to use them. Remember +//! that you will need to import this trait to access its methods. The easiest +//! way is to add `use rspotify::prelude::*`. //! //! * [`Type::Artist`] => [`ArtistId`] //! * [`Type::Album`] => [`AlbumId`] @@ -15,6 +16,9 @@ //! * [`Type::Show`] => [`ShowId`] //! * [`Type::Episode`] => [`EpisodeId`] //! +//! Every kind of ID defines its own validity function, i.e., what characters it +//! can be made up of, such as alphanumeric or any. +//! //! These types are just wrappers for [`Cow`], so their usage should be //! quite similar overall. //! @@ -25,8 +29,7 @@ //! If an endpoint requires a `TrackId`, you may pass it as: //! //! ``` -//! use rspotify_model::{Id, TrackId}; -//! +//! # use rspotify_model::TrackId; //! fn pause_track(id: TrackId<'_>) { /* ... */ } //! //! let id = TrackId::from_id("4iV5W9uYEdYUVa79Axb7Rh").unwrap(); @@ -37,8 +40,7 @@ //! compile-time: //! //! ```compile_fail -//! use rspotify_model::{Id, TrackId, EpisodeId}; -//! +//! # use rspotify_model::{TrackId, EpisodeId}; //! fn pause_track(id: TrackId<'_>) { /* ... */ } //! //! let id = EpisodeId::from_id("4iV5W9uYEdYUVa79Axb7Rh").unwrap(); @@ -49,8 +51,7 @@ //! it's an album (`spotify:album:xxxx`). //! //! ```should_panic -//! use rspotify_model::{Id, TrackId}; -//! +//! # use rspotify_model::TrackId; //! fn pause_track(id: TrackId<'_>) { /* ... */ } //! //! let id = TrackId::from_uri("spotify:album:6akEvsycLGftJxYudPjmqK").unwrap(); @@ -61,7 +62,7 @@ //! types: //! //! ``` -//! use rspotify_model::{Id, TrackId, EpisodeId, PlayableId}; +//! use rspotify_model::{TrackId, EpisodeId, PlayableId}; //! //! fn track(id: TrackId<'_>) { /* ... */ } //! fn episode(id: EpisodeId<'_>) { /* ... */ } @@ -103,7 +104,7 @@ use std::hash::Hash; use crate::Type; -/// Spotify id or URI parsing error +/// Spotify ID or URI parsing error /// /// See also [`Id`](crate::idtypes::Id) for details. #[derive(Debug, PartialEq, Eq, Clone, Copy, Display, Error)] @@ -120,13 +121,14 @@ pub enum IdError { InvalidId, } -/// The main interface for an ID. See the [`module level documentation`] for -/// more information. +/// The main interface for an ID. +/// +/// See the [module level documentation] for more information. /// -/// [`module level documentation`]: [`crate::idtypes`] +/// [module level documentation]: [`crate::idtypes`] #[enum_dispatch] pub trait Id<'a> { - /// Returns the inner Spotify object Id, which is guaranteed to be valid for + /// Returns the inner Spotify object ID, which is guaranteed to be valid for /// its type. fn id(&self) -> &str; @@ -150,11 +152,11 @@ pub trait Id<'a> { } } -/// A lower level function to parse a URI into both its type and its actual Id. -/// Note that this function doesn't check the validity of the returned Id, e.g., -/// if that it's alphanumeric; that should be done in `Id::from_id`. +/// A lower level function to parse a URI into both its type and its actual ID. +/// Note that this function doesn't check the validity of the returned ID (e.g., +/// whether it's alphanumeric; that should be done in `Id::from_id`). /// -/// This is only useful for advanced cases, such as implementing your own Id +/// This is only useful for advanced use-cases, such as implementing your own ID /// type. pub fn parse_uri(uri: &str) -> Result<(Type, &str), IdError> { let mut chars = uri @@ -197,9 +199,9 @@ macro_rules! define_idtypes { }),+) => { $( #[doc = concat!( - "ID of type [`Type::", stringify!($type), "`]. Its \ - implementation of `id_is_valid` is defined by the closure `", - stringify!($validity), "`.\nRefer to the [module-level \ + "ID of type [`Type::", stringify!($type), "`]. The validity of \ + its characters is defined by the closure `", + stringify!($validity), "`.\n\nRefer to the [module-level \ docs][`crate::idtypes`] for more information. " )] #[repr(transparent)] @@ -207,21 +209,23 @@ macro_rules! define_idtypes { pub struct $name<'a>(::std::borrow::Cow<'a, str>); impl<'a> $name<'a> { + /// The type of the ID, as a constant. + const TYPE: Type = Type::$type; + + /// An ID is a `Cow` after all, so this will switch to the its + /// owned version, which has a `'static` lifetime. pub fn into_static(self) -> $name<'static> { $name(self.0.to_string().into()) } + /// Similar to [`Self::into_static`], but without consuming the + /// original ID. pub fn clone_static(&self) -> $name<'static> { $name(self.0.to_string().into()) } - } - - impl<'a> $name<'a> { - /// The type of the Id, as a constant. - const TYPE: Type = Type::$type; /// Only returns `true` in case the given string is valid - /// according to that specific Id (e.g., some may require + /// according to that specific ID (e.g., some may require /// alphanumeric characters only). #[inline] pub fn id_is_valid(id: &str) -> bool { @@ -230,7 +234,7 @@ macro_rules! define_idtypes { VALID_FN(id) } - /// Initialize the Id without checking its validity. + /// Initialize the ID without checking its validity. /// /// # Safety /// @@ -241,7 +245,7 @@ macro_rules! define_idtypes { Self(std::borrow::Cow::Borrowed(id)) } - /// Parse Spotify Id from string slice. + /// Parse Spotify ID from string slice. /// /// A valid Spotify object id must be a non-empty string with /// valid characters. @@ -251,7 +255,7 @@ macro_rules! define_idtypes { /// - `IdError::InvalidId` - if `id` contains invalid characters. pub fn from_id(id: &'a str) -> Result { if Self::id_is_valid(id) { - // Safe, we've just checked that the Id is valid. + // Safe, we've just checked that the ID is valid. Ok(unsafe { Self::from_id_unchecked(id) }) } else { Err(IdError::InvalidId) @@ -288,7 +292,7 @@ macro_rules! define_idtypes { } } - /// Parse Spotify id or URI from string slice + /// Parse Spotify ID or URI from string slice /// /// Spotify URI must be in one of the following formats: /// `spotify:{type}:{id}` or `spotify/{type}/{id}`. @@ -323,7 +327,7 @@ macro_rules! define_idtypes { } // TODO: better explanation? - /// Useful to keep using an Id without having to clone it. + /// Useful to keep using an ID without having to clone it. #[inline] pub fn as_ref(&'a self) -> $name<'a> { Self(std::borrow::Cow::Borrowed(self.0.as_ref())) @@ -342,7 +346,7 @@ macro_rules! define_idtypes { } } - // Deserialization may take either an Id or an URI, so its + // Deserialization may take either an ID or an URI, so its // implementation has to be done manually. impl<'de> Deserialize<'de> for $name<'static> { fn deserialize(deserializer: D) -> Result @@ -454,9 +458,10 @@ define_idtypes!( } ); -// Grouping up the IDs into more specific traits with the help of -// `enum_dispatch`, which is not only easier to use than `dyn`, but also more -// efficient. +// We use `enum_dispatch` for dynamic dispatch, which is not only easier to use +// than `dyn`, but also more efficient. +/// Grouping up multiple kinds of IDs to treat them generically. This also +/// implements [`Id`], and [`From`] to instantiate it. #[enum_dispatch(Id)] pub enum PlayContextId<'a> { Artist(ArtistId<'a>), @@ -477,6 +482,8 @@ impl<'a> PlayContextId<'a> { } } +/// Grouping up multiple kinds of IDs to treat them generically. This also +/// implements [`Id`] and [`From`] to instantiate it. #[enum_dispatch(Id)] pub enum PlayableId<'a> { Track(TrackId<'a>), From 5613da3e98444d7d778f18616e4e950c8a840d7c Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sat, 11 Jun 2022 16:29:23 +0200 Subject: [PATCH 14/28] Resolve all that was left * Remove `inline`: https://matklad.github.io/2021/07/09/inline-in-rust.html * Fix documentation * Use `into_owned` --- rspotify-model/src/idtypes.rs | 52 +++++++++++++---------------------- tests/test_with_credential.rs | 5 +--- tests/test_with_oauth.rs | 48 ++++++++++++++++++++++++++------ 3 files changed, 59 insertions(+), 46 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index a2e3f8d2..d39f68dc 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -184,14 +184,10 @@ pub fn parse_uri(uri: &str) -> Result<(Type, &str), IdError> { /// This macro helps consistently define ID types. /// -/// * The `$type` parameter indicates what type the ID is made out of (say, -/// `Artist`, `Album`...) from the enum `Type`. -/// * The `$name` parameter is the identifier of the struct for that type. -/// -/// This macro contains a lot of code but mostly it's just repetitive work to -/// implement some common traits that's not of much interest for being trivial. -/// -/// * The `$name` parameter is the identifier of the struct for that type. +/// * The `$type` parameter indicates what variant in `Type` the ID is for (say, +/// `Artist`, or `Album`). +/// * The `$name` parameter is the identifier of the struct. +/// * The `$validity` parameter is the implementation of `is_id_valid`. macro_rules! define_idtypes { ($($type:ident => { name: $name:ident, @@ -212,24 +208,10 @@ macro_rules! define_idtypes { /// The type of the ID, as a constant. const TYPE: Type = Type::$type; - /// An ID is a `Cow` after all, so this will switch to the its - /// owned version, which has a `'static` lifetime. - pub fn into_static(self) -> $name<'static> { - $name(self.0.to_string().into()) - } - - /// Similar to [`Self::into_static`], but without consuming the - /// original ID. - pub fn clone_static(&self) -> $name<'static> { - $name(self.0.to_string().into()) - } - /// Only returns `true` in case the given string is valid /// according to that specific ID (e.g., some may require /// alphanumeric characters only). - #[inline] pub fn id_is_valid(id: &str) -> bool { - // TODO: make prettier? const VALID_FN: fn(&str) -> bool = $validity; VALID_FN(id) } @@ -240,7 +222,6 @@ macro_rules! define_idtypes { /// /// The string passed to this method must be made out of valid /// characters only; otherwise undefined behaviour may occur. - #[inline] pub unsafe fn from_id_unchecked(id: &'a str) -> Self { Self(std::borrow::Cow::Borrowed(id)) } @@ -326,21 +307,31 @@ macro_rules! define_idtypes { } } - // TODO: better explanation? - /// Useful to keep using an ID without having to clone it. - #[inline] + /// This creates an ID with the underlying `&str` variant from a + /// reference. Useful to use an ID multiple times without having + /// to clone it. pub fn as_ref(&'a self) -> $name<'a> { Self(std::borrow::Cow::Borrowed(self.0.as_ref())) } + + /// An ID is a `Cow` after all, so this will switch to the its + /// owned version, which has a `'static` lifetime. + pub fn into_static(self) -> $name<'static> { + $name(self.0.into_owned().into()) + } + + /// Similar to [`Self::into_static`], but without consuming the + /// original ID. + pub fn clone_static(&self) -> $name<'static> { + $name(self.0.clone().into_owned().into()) + } } impl<'a> Id<'a> for $name<'a> { - #[inline] fn id(&self) -> &str { &self.0 } - #[inline] fn _type(&self) -> Type { Self::TYPE } @@ -366,7 +357,6 @@ macro_rules! define_idtypes { formatter.write_str(msg) } - #[inline] fn visit_str(self, value: &str) -> Result where E: serde::de::Error, @@ -376,7 +366,6 @@ macro_rules! define_idtypes { .map_err(serde::de::Error::custom) } - #[inline] fn visit_newtype_struct( self, deserializer: A, @@ -387,7 +376,6 @@ macro_rules! define_idtypes { deserializer.deserialize_str(self) } - #[inline] fn visit_seq( self, mut seq: A, @@ -471,7 +459,6 @@ pub enum PlayContextId<'a> { } // These don't work with `enum_dispatch`, unfortunately. impl<'a> PlayContextId<'a> { - #[inline] pub fn as_ref(&'a self) -> PlayContextId<'a> { match self { PlayContextId::Artist(x) => PlayContextId::Artist(x.as_ref()), @@ -491,7 +478,6 @@ pub enum PlayableId<'a> { } // These don't work with `enum_dispatch`, unfortunately. impl<'a> PlayableId<'a> { - #[inline] pub fn as_ref(&'a self) -> PlayableId<'a> { match self { PlayableId::Track(x) => PlayableId::Track(x.as_ref()), diff --git a/tests/test_with_credential.rs b/tests/test_with_credential.rs index 151a3968..1bab598d 100644 --- a/tests/test_with_credential.rs +++ b/tests/test_with_credential.rs @@ -160,10 +160,7 @@ async fn test_existing_playlist() { #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] async fn test_fake_playlist() { let playlist_id = PlaylistId::from_id("fakeid").unwrap(); - let playlist = creds_client() - .await - .playlist(playlist_id, None, None) - .await; + let playlist = creds_client().await.playlist(playlist_id, None, None).await; assert!(playlist.is_err()); } diff --git a/tests/test_with_oauth.rs b/tests/test_with_oauth.rs index c083db06..16443232 100644 --- a/tests/test_with_oauth.rs +++ b/tests/test_with_oauth.rs @@ -565,7 +565,10 @@ async fn test_user_follow_artist() { ArtistId::from_id("08td7MxkoHQkXnWAYD8d6Q").unwrap(), ]; - client.user_follow_artists(artists.iter().map(ArtistId::as_ref)).await.unwrap(); + client + .user_follow_artists(artists.iter().map(ArtistId::as_ref)) + .await + .unwrap(); client.user_unfollow_artists(artists).await.unwrap(); } @@ -578,7 +581,10 @@ async fn test_user_follow_users() { UserId::from_id("john").unwrap(), ]; - client.user_follow_users(users.iter().map(UserId::as_ref)).await.unwrap(); + client + .user_follow_users(users.iter().map(UserId::as_ref)) + .await + .unwrap(); client.user_unfollow_users(users).await.unwrap(); } @@ -652,7 +658,11 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist // Firstly adding some tracks client - .playlist_add_items(playlist.id.as_ref(), tracks.iter().map(PlayableId::as_ref), None) + .playlist_add_items( + playlist.id.as_ref(), + tracks.iter().map(PlayableId::as_ref), + None, + ) .await .unwrap(); check_num_tracks(client, playlist.id.as_ref(), tracks.len() as i32).await; @@ -676,7 +686,10 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist PlayableId::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), ]; client - .playlist_replace_items(playlist.id.as_ref(), replaced_tracks.iter().map(|t| t.as_ref())) + .playlist_replace_items( + playlist.id.as_ref(), + replaced_tracks.iter().map(|t| t.as_ref()), + ) .await .unwrap(); // Making sure the number of tracks is updated @@ -685,11 +698,15 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist // Removes a few specific tracks let tracks = [ ItemPositions { - id: PlayableId::Track(TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap()), + id: PlayableId::Track( + TrackId::from_uri("spotify:track:4iV5W9uYEdYUVa79Axb7Rh").unwrap(), + ), positions: &[0], }, ItemPositions { - id: PlayableId::Track(TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap()), + id: PlayableId::Track( + TrackId::from_uri("spotify:track:5m2en2ndANCPembKOYr1xL").unwrap(), + ), positions: &[4, 6], }, ]; @@ -698,7 +715,12 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist .await .unwrap(); // Making sure three tracks were removed - check_num_tracks(client, playlist.id.as_ref(), replaced_tracks.len() as i32 - 3).await; + check_num_tracks( + client, + playlist.id.as_ref(), + replaced_tracks.len() as i32 - 3, + ) + .await; // Removes all occurrences of two tracks let to_remove = vec![ @@ -710,7 +732,12 @@ async fn check_playlist_tracks(client: &AuthCodeSpotify, playlist: &FullPlaylist .await .unwrap(); // Making sure two more tracks were removed - check_num_tracks(client, playlist.id.as_ref(), replaced_tracks.len() as i32 - 5).await; + check_num_tracks( + client, + playlist.id.as_ref(), + replaced_tracks.len() as i32 - 5, + ) + .await; } #[maybe_async] @@ -728,7 +755,10 @@ async fn check_playlist_follow(client: &AuthCodeSpotify, playlist: &FullPlaylist assert_eq!(following, vec![false, false]); // Finally unfollowing the playlist in order to clean it up - client.playlist_unfollow(playlist.id.as_ref()).await.unwrap(); + client + .playlist_unfollow(playlist.id.as_ref()) + .await + .unwrap(); } #[maybe_async::test(feature = "__sync", async(feature = "__async", tokio::test))] From eb6c1e4c1548c58b92806d8ae954754bfe41eca7 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sat, 11 Jun 2022 16:44:44 +0200 Subject: [PATCH 15/28] Fix tests and string interpolation --- examples/auth_code.rs | 2 +- examples/auth_code_pkce.rs | 4 ++-- examples/client_creds.rs | 4 ++-- examples/tasks.rs | 2 +- examples/ureq/device.rs | 2 +- examples/ureq/me.rs | 2 +- examples/ureq/search.rs | 24 ++++++++++++------------ examples/ureq/seek_track.rs | 4 ++-- examples/ureq/threading.rs | 2 +- examples/with_auto_reauth.rs | 15 ++++++++------- examples/with_refresh_token.rs | 13 +++++++------ tests/test_with_credential.rs | 2 +- 12 files changed, 39 insertions(+), 37 deletions(-) diff --git a/examples/auth_code.rs b/examples/auth_code.rs index 5db273de..5f5b655a 100644 --- a/examples/auth_code.rs +++ b/examples/auth_code.rs @@ -49,5 +49,5 @@ async fn main() { .current_playing(Some(&market), Some(&additional_types)) .await; - println!("Response: {:?}", artists); + println!("Response: {artists:?}"); } diff --git a/examples/auth_code_pkce.rs b/examples/auth_code_pkce.rs index 239469cb..fb3164ef 100644 --- a/examples/auth_code_pkce.rs +++ b/examples/auth_code_pkce.rs @@ -39,7 +39,7 @@ async fn main() { // Running the requests let history = spotify.current_playback(None, None::>).await; - println!("Response: {:?}", history); + println!("Response: {history:?}"); // Token refreshing works as well, but only with the one generated in the // previous request (they actually expire, unlike the regular code auth @@ -51,5 +51,5 @@ async fn main() { // Running the requests again let history = spotify.current_playback(None, None::>).await; - println!("Response after refreshing token: {:?}", history); + println!("Response after refreshing token: {history:?}"); } diff --git a/examples/client_creds.rs b/examples/client_creds.rs index 22aee06e..e91069a3 100644 --- a/examples/client_creds.rs +++ b/examples/client_creds.rs @@ -33,7 +33,7 @@ async fn main() { // Running the requests let birdy_uri = AlbumId::from_uri("spotify:album:0sNOF9WDwhWunNAHPD3Baj").unwrap(); - let albums = spotify.album(&birdy_uri).await; + let albums = spotify.album(birdy_uri).await; - println!("Response: {:#?}", albums); + println!("Response: {albums:#?}"); } diff --git a/examples/tasks.rs b/examples/tasks.rs index cc03880b..53dda232 100644 --- a/examples/tasks.rs +++ b/examples/tasks.rs @@ -28,7 +28,7 @@ async fn main() { let spotify = Arc::clone(&spotify); let wr = wr.clone(); let handle = task::spawn(async move { - let albums = spotify.album(&id).await.unwrap(); + let albums = spotify.album(id).await.unwrap(); wr.send(albums).unwrap(); }); diff --git a/examples/ureq/device.rs b/examples/ureq/device.rs index 9b0192cf..2d9f89b1 100644 --- a/examples/ureq/device.rs +++ b/examples/ureq/device.rs @@ -18,5 +18,5 @@ fn main() { let devices = spotify.device(); - println!("Request: {:?}", devices); + println!("Request: {devices:?}"); } diff --git a/examples/ureq/me.rs b/examples/ureq/me.rs index dc508bc5..08dc4355 100644 --- a/examples/ureq/me.rs +++ b/examples/ureq/me.rs @@ -17,5 +17,5 @@ fn main() { spotify.prompt_for_token(&url).unwrap(); let user = spotify.me(); - println!("Request: {:?}", user); + println!("Request: {user:?}"); } diff --git a/examples/ureq/search.rs b/examples/ureq/search.rs index 2d423a6e..10fc2d06 100644 --- a/examples/ureq/search.rs +++ b/examples/ureq/search.rs @@ -19,8 +19,8 @@ fn main() { let album_query = "album:arrival artist:abba"; let result = spotify.search(album_query, &SearchType::Album, None, None, Some(10), None); match result { - Ok(album) => println!("searched album:{:?}", album), - Err(err) => println!("search error!{:?}", err), + Ok(album) => println!("Searched album: {album:?}"), + Err(err) => println!("Search error! {err:?}"), } let artist_query = "tania bowra"; @@ -33,8 +33,8 @@ fn main() { None, ); match result { - Ok(album) => println!("searched artist:{:?}", album), - Err(err) => println!("search error!{:?}", err), + Ok(album) => println!("Searched artist: {album:?}"), + Err(err) => println!("Search error! {err:?}"), } let playlist_query = "\"doom metal\""; @@ -47,8 +47,8 @@ fn main() { None, ); match result { - Ok(album) => println!("searched playlist:{:?}", album), - Err(err) => println!("search error!{:?}", err), + Ok(album) => println!("Searched playlist: {album:?}"), + Err(err) => println!("Search error! {err:?}"), } let track_query = "abba"; @@ -61,15 +61,15 @@ fn main() { None, ); match result { - Ok(album) => println!("searched track:{:?}", album), - Err(err) => println!("search error!{:?}", err), + Ok(album) => println!("Searched track: {album:?}"), + Err(err) => println!("Search error! {err:?}"), } let show_query = "love"; let result = spotify.search(show_query, &SearchType::Show, None, None, Some(10), None); match result { - Ok(show) => println!("searched show:{:?}", show), - Err(err) => println!("search error!{:?}", err), + Ok(show) => println!("Searched show: {show:?}"), + Err(err) => println!("Search error! {err:?}"), } let episode_query = "love"; @@ -82,7 +82,7 @@ fn main() { None, ); match result { - Ok(episode) => println!("searched episode:{:?}", episode), - Err(err) => println!("search error!{:?}", err), + Ok(episode) => println!("Searched episode: {episode:?}"), + Err(err) => println!("Search error! {err:?}"), } } diff --git a/examples/ureq/seek_track.rs b/examples/ureq/seek_track.rs index 11763790..b4907954 100644 --- a/examples/ureq/seek_track.rs +++ b/examples/ureq/seek_track.rs @@ -17,7 +17,7 @@ fn main() { spotify.prompt_for_token(&url).unwrap(); match spotify.seek_track(25000, None) { - Ok(_) => println!("change to previous playback successful"), - Err(_) => eprintln!("change to previous playback failed"), + Ok(_) => println!("Change to previous playback successful"), + Err(_) => eprintln!("Change to previous playback failed"), } } diff --git a/examples/ureq/threading.rs b/examples/ureq/threading.rs index 6d3f544b..1abf39ee 100644 --- a/examples/ureq/threading.rs +++ b/examples/ureq/threading.rs @@ -29,7 +29,7 @@ fn main() { let spotify = Arc::clone(&spotify); let wr = wr.clone(); let handle = thread::spawn(move || { - let albums = spotify.album(&id).unwrap(); + let albums = spotify.album(id).unwrap(); wr.send(albums).unwrap(); }); diff --git a/examples/with_auto_reauth.rs b/examples/with_auto_reauth.rs index 57e3b80e..95ecc6ea 100644 --- a/examples/with_auto_reauth.rs +++ b/examples/with_auto_reauth.rs @@ -14,15 +14,16 @@ use rspotify::{ // followed artists, and then unfollow the artists. async fn auth_code_do_things(spotify: &AuthCodeSpotify) { let artists = [ - &ArtistId::from_id("3RGLhK1IP9jnYFH4BRFJBS").unwrap(), // The Clash - &ArtistId::from_id("0yNLKJebCb8Aueb54LYya3").unwrap(), // New Order - &ArtistId::from_id("2jzc5TC5TVFLXQlBNiIUzE").unwrap(), // a-ha + ArtistId::from_id("3RGLhK1IP9jnYFH4BRFJBS").unwrap(), // The Clash + ArtistId::from_id("0yNLKJebCb8Aueb54LYya3").unwrap(), // New Order + ArtistId::from_id("2jzc5TC5TVFLXQlBNiIUzE").unwrap(), // a-ha ]; + let num_artists = artists.len(); spotify - .user_follow_artists(artists) + .user_follow_artists(artists.iter().map(|a| a.as_ref())) .await .expect("couldn't follow artists"); - println!("Followed {} artists successfully.", artists.len()); + println!("Followed {num_artists} artists successfully."); // Printing the followed artists let followed = spotify @@ -38,13 +39,13 @@ async fn auth_code_do_things(spotify: &AuthCodeSpotify) { .user_unfollow_artists(artists) .await .expect("couldn't unfollow artists"); - println!("Unfollowed {} artists successfully.", artists.len()); + println!("Unfollowed {num_artists} artists successfully."); } async fn client_creds_do_things(spotify: &ClientCredsSpotify) { // Running the requests let birdy_uri = AlbumId::from_uri("spotify:album:0sNOF9WDwhWunNAHPD3Baj").unwrap(); - let albums = spotify.album(&birdy_uri).await; + let albums = spotify.album(birdy_uri).await; println!("Get albums: {}", albums.unwrap().id); } diff --git a/examples/with_refresh_token.rs b/examples/with_refresh_token.rs index 54063df6..f2f3ef13 100644 --- a/examples/with_refresh_token.rs +++ b/examples/with_refresh_token.rs @@ -21,15 +21,16 @@ use rspotify::{model::ArtistId, prelude::*, scopes, AuthCodeSpotify, Credentials // followed artists, and then unfollow the artists. async fn do_things(spotify: AuthCodeSpotify) { let artists = [ - &ArtistId::from_id("3RGLhK1IP9jnYFH4BRFJBS").unwrap(), // The Clash - &ArtistId::from_id("0yNLKJebCb8Aueb54LYya3").unwrap(), // New Order - &ArtistId::from_id("2jzc5TC5TVFLXQlBNiIUzE").unwrap(), // a-ha + ArtistId::from_id("3RGLhK1IP9jnYFH4BRFJBS").unwrap(), // The Clash + ArtistId::from_id("0yNLKJebCb8Aueb54LYya3").unwrap(), // New Order + ArtistId::from_id("2jzc5TC5TVFLXQlBNiIUzE").unwrap(), // a-ha ]; + let num_artists = artists.len(); spotify - .user_follow_artists(artists) + .user_follow_artists(artists.iter().map(|a| a.as_ref())) .await .expect("couldn't follow artists"); - println!("Followed {} artists successfully.", artists.len()); + println!("Followed {num_artists} artists successfully."); // Printing the followed artists let followed = spotify @@ -45,7 +46,7 @@ async fn do_things(spotify: AuthCodeSpotify) { .user_unfollow_artists(artists) .await .expect("couldn't unfollow artists"); - println!("Unfollowed {} artists successfully.", artists.len()); + println!("Unfollowed {num_artists} artists successfully."); } #[tokio::main] diff --git a/tests/test_with_credential.rs b/tests/test_with_credential.rs index 1bab598d..168ccdfe 100644 --- a/tests/test_with_credential.rs +++ b/tests/test_with_credential.rs @@ -1,5 +1,5 @@ use rspotify::{ - model::{AlbumId, AlbumType, ArtistId, Country, Id, Market, PlaylistId, TrackId, UserId}, + model::{AlbumId, AlbumType, ArtistId, Country, Market, PlaylistId, TrackId, UserId}, prelude::*, ClientCredsSpotify, Credentials, }; From c50235b559353dc286d70fc959372eb64a2470ac Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sat, 11 Jun 2022 16:56:11 +0200 Subject: [PATCH 16/28] Webapp example compiles now --- examples/webapp/Cargo.toml | 11 +++++++---- examples/webapp/src/main.rs | 17 +++++++++-------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/examples/webapp/Cargo.toml b/examples/webapp/Cargo.toml index 9802eea7..4fe19d24 100644 --- a/examples/webapp/Cargo.toml +++ b/examples/webapp/Cargo.toml @@ -1,12 +1,15 @@ [package] name = "webapp" version = "0.1.0" -authors = ["Ramsay "] +authors = [ + "Ramsay Leung ", + "Mario Ortiz Manero " +] edition = "2018" [dependencies] -rocket = "0.4.5" -rocket_contrib = { version = "0.4.5", features = ["tera_templates"] } -getrandom = "0.2.0" +rocket = "0.4.10" +rocket_contrib = { version = "0.4.10", features = ["tera_templates"] } +getrandom = "0.2.6" # Rocket is synchronous, so this uses the `ureq` client rspotify = { path = "../..", features = ["client-ureq", "ureq-rustls-tls"], default-features = false } diff --git a/examples/webapp/src/main.rs b/examples/webapp/src/main.rs index e2bf55df..78d13d0b 100644 --- a/examples/webapp/src/main.rs +++ b/examples/webapp/src/main.rs @@ -14,13 +14,13 @@ use rocket::response::Redirect; use rocket_contrib::json; use rocket_contrib::json::JsonValue; use rocket_contrib::templates::Template; -use rspotify::{scopes, AuthCodeSpotify, OAuth, Credentials, Config, prelude::*, Token}; +use rspotify::{prelude::*, scopes, AuthCodeSpotify, Config, Credentials, OAuth, Token}; -use std::fs; use std::{ collections::HashMap, - env, + env, fs, path::PathBuf, + sync::{Arc, Mutex}, }; #[derive(Debug, Responder)] @@ -95,7 +95,7 @@ fn init_spotify(cookies: &Cookies) -> AuthCodeSpotify { // Replacing client_id and client_secret with yours. let creds = Credentials::new( "e1dce60f1e274e20861ce5d96142a4d3", - "0e4e03b9be8d465d87fc32857a4b5aa3" + "0e4e03b9be8d465d87fc32857a4b5aa3", ); AuthCodeSpotify::with_config(creds, oauth, config) @@ -168,9 +168,10 @@ fn playlist(cookies: Cookies) -> AppResponse { return AppResponse::Redirect(Redirect::to("/")); } - let token = spotify.read_token_cache().unwrap(); - spotify.token = Some(token); - let playlists = spotify.current_user_playlists() + let token = spotify.read_token_cache(false).unwrap(); + spotify.token = Arc::new(Mutex::new(token)); + let playlists = spotify + .current_user_playlists() .take(50) .filter_map(Result::ok) .collect::>(); @@ -189,7 +190,7 @@ fn me(cookies: Cookies) -> AppResponse { return AppResponse::Redirect(Redirect::to("/")); } - spotify.token = Some(spotify.read_token_cache().unwrap()); + spotify.token = Arc::new(Mutex::new(spotify.read_token_cache(false).unwrap())); match spotify.me() { Ok(user_info) => AppResponse::Json(json!(user_info)), Err(_) => AppResponse::Redirect(Redirect::to("/")), From 8ae1d22daa548553aca1f83c6a90b89f0010a1a0 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sat, 11 Jun 2022 17:23:35 +0200 Subject: [PATCH 17/28] Attempt at generic constructors --- rspotify-model/src/idtypes.rs | 76 +++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 25 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index d39f68dc..10fbed71 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -99,8 +99,7 @@ use serde::{Deserialize, Serialize}; use strum::Display; use thiserror::Error; -use std::fmt::Debug; -use std::hash::Hash; +use std::{borrow::Cow, fmt::Debug, hash::Hash}; use crate::Type; @@ -202,7 +201,7 @@ macro_rules! define_idtypes { )] #[repr(transparent)] #[derive(Clone, Debug, PartialEq, Eq, Serialize, Hash)] - pub struct $name<'a>(::std::borrow::Cow<'a, str>); + pub struct $name<'a>(Cow<'a, str>); impl<'a> $name<'a> { /// The type of the ID, as a constant. @@ -222,8 +221,11 @@ macro_rules! define_idtypes { /// /// The string passed to this method must be made out of valid /// characters only; otherwise undefined behaviour may occur. - pub unsafe fn from_id_unchecked(id: &'a str) -> Self { - Self(std::borrow::Cow::Borrowed(id)) + pub unsafe fn from_id_unchecked(id: S) -> Self + where + S: Into> + { + Self(id.into()) } /// Parse Spotify ID from string slice. @@ -234,8 +236,12 @@ macro_rules! define_idtypes { /// # Errors: /// /// - `IdError::InvalidId` - if `id` contains invalid characters. - pub fn from_id(id: &'a str) -> Result { - if Self::id_is_valid(id) { + pub fn from_id(id: S) -> Result + where + S: Into> + { + let id = id.into(); + if Self::id_is_valid(&id) { // Safe, we've just checked that the ID is valid. Ok(unsafe { Self::from_id_unchecked(id) }) } else { @@ -264,8 +270,12 @@ macro_rules! define_idtypes { /// valid id, /// - `IdError::InvalidFormat` - if it can't be splitted into /// type and id parts. - pub fn from_uri(uri: &'a str) -> Result { - let (tpe, id) = parse_uri(uri)?; + pub fn from_uri(uri: S) -> Result + where + S: Into> + { + let uri = uri.into(); + let (tpe, id) = parse_uri(&uri)?; if tpe == Type::$type { Self::from_id(id) } else { @@ -299,8 +309,12 @@ macro_rules! define_idtypes { /// if it contains valid characters), /// - `IdError::InvalidFormat` - if `id_or_uri` is an URI, and /// it can't be split into type and id parts. - pub fn from_id_or_uri(id_or_uri: &'a str) -> Result { - match Self::from_uri(id_or_uri) { + pub fn from_id_or_uri(id_or_uri: S) -> Result + where + S: Into> + { + let id_or_uri = id_or_uri.into(); + match Self::from_uri(id_or_uri.clone()) { Ok(id) => Ok(id), Err(IdError::InvalidPrefix) => Self::from_id(id_or_uri), Err(error) => Err(error), @@ -311,7 +325,7 @@ macro_rules! define_idtypes { /// reference. Useful to use an ID multiple times without having /// to clone it. pub fn as_ref(&'a self) -> $name<'a> { - Self(std::borrow::Cow::Borrowed(self.0.as_ref())) + Self(Cow::Borrowed(self.0.as_ref())) } /// An ID is a `Cow` after all, so this will switch to the its @@ -383,7 +397,7 @@ macro_rules! define_idtypes { where A: serde::de::SeqAccess<'de>, { - let field = seq.next_element()? + let field: &str = seq.next_element()? .ok_or_else(|| serde::de::Error::invalid_length(0, &self))?; $name::from_id_or_uri(field) .map($name::into_static) @@ -489,7 +503,7 @@ impl<'a> PlayableId<'a> { #[cfg(test)] mod test { use super::*; - use std::error::Error; + use std::{borrow::Cow, error::Error}; // Valid values: const ID: &str = "4iV5W9uYEdYUVa79Axb7Rh"; @@ -531,7 +545,7 @@ mod test { fn test_id_or_uri_and_deserialize() { fn test_any(check: F) where - F: Fn(&str) -> Result<&TrackId, E>, + F: Fn(&str) -> Result, E>, E: Error, { // In this case we also check that the contents are the ID and not @@ -570,29 +584,41 @@ mod test { #[test] fn test_multiple_types() { - fn endpoint(_ids: impl IntoIterator) {} + fn endpoint<'a>(_ids: impl IntoIterator>) {} - let tracks: Vec = vec![ - Playable::Track(TrackId::from_id(ID).unwrap()), - Playable::Track(TrackId::from_id(ID).unwrap()), - Playable::Episode(EpisodeId::from_id(ID).unwrap()), - Playable::Episode(EpisodeId::from_id(ID).unwrap()), + let tracks: Vec = vec![ + PlayableId::Track(TrackId::from_id(ID).unwrap()), + PlayableId::Track(TrackId::from_id(ID).unwrap()), + PlayableId::Episode(EpisodeId::from_id(ID).unwrap()), + PlayableId::Episode(EpisodeId::from_id(ID).unwrap()), ]; endpoint(tracks); } #[test] fn test_unknown_at_compile_time() { - fn endpoint1(input: &str, is_episode: bool) -> Playable { + fn endpoint1(input: &str, is_episode: bool) -> PlayableId<'_> { if is_episode { - Playable::Episode(EpisodeId::from_id(input).unwrap()) + PlayableId::Episode(EpisodeId::from_id(input).unwrap()) } else { - Playable::Track(TrackId::from_id(input).unwrap()) + PlayableId::Track(TrackId::from_id(input).unwrap()) } } - fn endpoint2(_id: &[Playable]) {} + fn endpoint2(_id: &[PlayableId]) {} let id = endpoint1(ID, false); endpoint2(&[id]); } + + #[test] + fn test_constructor() { + // With `&str` + let _ = EpisodeId::from_id(ID).unwrap(); + // With `String` + let _ = EpisodeId::from_id(ID.to_string()).unwrap(); + // With borrowed `Cow` + let _ = EpisodeId::from_id(Cow::Borrowed(ID)).unwrap(); + // With owned `Cow` + let _ = EpisodeId::from_id(Cow::Owned(ID.to_string())).unwrap(); + } } From 2911b0f3f39ee760781d188c1c1a44f91c908f6f Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Fri, 17 Jun 2022 15:50:50 +0200 Subject: [PATCH 18/28] Revert parameter to &str --- rspotify-model/src/idtypes.rs | 37 +++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 10fbed71..7b4f6213 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -233,7 +233,7 @@ macro_rules! define_idtypes { /// A valid Spotify object id must be a non-empty string with /// valid characters. /// - /// # Errors: + /// # Errors /// /// - `IdError::InvalidId` - if `id` contains invalid characters. pub fn from_id(id: S) -> Result @@ -260,7 +260,7 @@ macro_rules! define_idtypes { /// Examples: `spotify:album:6IcGNaXFRf5Y1jc7QsE9O2`, /// `spotify/track/4y4VO05kYgUTo2bzbox1an`. /// - /// # Errors: + /// # Errors /// /// - `IdError::InvalidPrefix` - if `uri` is not started with /// `spotify:` or `spotify/`, @@ -270,11 +270,16 @@ macro_rules! define_idtypes { /// valid id, /// - `IdError::InvalidFormat` - if it can't be splitted into /// type and id parts. - pub fn from_uri(uri: S) -> Result - where - S: Into> - { - let uri = uri.into(); + /// + /// # Implementation details + /// + /// Unlike [`Self::from_id`], this method takes a `&str` rather + /// than an `Into>`. This is because the inner `Cow` in + /// the ID would reference a slice from the given `&str` (i.e., + /// taking the ID out of the URI). The parameter wouldn't live + /// long enough when using `Into>`, so the only + /// sensible choice is to just use a `&str`. + pub fn from_uri(uri: &'a str) -> Result { let (tpe, id) = parse_uri(&uri)?; if tpe == Type::$type { Self::from_id(id) @@ -300,7 +305,7 @@ macro_rules! define_idtypes { /// with `spotify:` or `spotify/`), it must be a valid Spotify /// object ID, i.e. a non-empty valid string. /// - /// # Errors: + /// # Errors /// /// - `IdError::InvalidType` - if `id_or_uri` is an URI, and /// it's type part is not equal to `T`, @@ -309,12 +314,18 @@ macro_rules! define_idtypes { /// if it contains valid characters), /// - `IdError::InvalidFormat` - if `id_or_uri` is an URI, and /// it can't be split into type and id parts. - pub fn from_id_or_uri(id_or_uri: S) -> Result - where - S: Into> - { + /// + /// # Implementation details + /// + /// Unlike [`Self::from_id`], this method takes a `&str` rather + /// than an `Into>`. This is because the inner `Cow` in + /// the ID would reference a slice from the given `&str` (i.e., + /// taking the ID out of the URI). The parameter wouldn't live + /// long enough when using `Into>`, so the only + /// sensible choice is to just use a `&str`. + pub fn from_id_or_uri(id_or_uri: &'a str) -> Result { let id_or_uri = id_or_uri.into(); - match Self::from_uri(id_or_uri.clone()) { + match Self::from_uri(id_or_uri) { Ok(id) => Ok(id), Err(IdError::InvalidPrefix) => Self::from_id(id_or_uri), Err(error) => Err(error), From 8da83aff627afa8e034ee1adb2663fe41183c8d3 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Fri, 17 Jun 2022 15:57:16 +0200 Subject: [PATCH 19/28] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd2e2dc6..db2858f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## Unreleased **Breaking changes**: +- ([#305](https://github.com/ramsayleung/rspotify/pull/305)) The `Id` types have been refactored to maximize usability. Instead of focusing on having an object-safe trait and using `dyn`, we now have enums to group up the IDs. This is based on how [`enum_dispatch`](https://docs.rs/enum_dispatch) works, and it's not only easier to use, but also faster. It makes it possible to have borrowed IDs again, so we've chosen to use `Cow` internally for flexibility. Check out the docs for more information! - ([#325](https://github.com/ramsayleung/rspotify/pull/325)) The `auth_code`, `auth_code_pkce`, `client_creds`, `clients::base` and `clients::oauth` modules have been removed from the public API; you should access the same types from their parent modules instead - ([#326](https://github.com/ramsayleung/rspotify/pull/326)) The `rspotify::clients::mutex` module has been renamed to `rspotify::sync` - ([#330](https://github.com/ramsayleung/rspotify/pull/330)) `search` now accepts `Option` instead of `Option<&IncludeExternal>` From 8019a5402cbbffb48f840ba0292e97173fea1aca Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Fri, 17 Jun 2022 15:58:20 +0200 Subject: [PATCH 20/28] Fix clippy --- tests/test_with_credential.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_with_credential.rs b/tests/test_with_credential.rs index 168ccdfe..6f08b989 100644 --- a/tests/test_with_credential.rs +++ b/tests/test_with_credential.rs @@ -190,7 +190,7 @@ mod test_pagination { let album = AlbumId::from_uri(ALBUM).unwrap(); let names = client - .album_track(&album) + .album_track(album) .map(|track| track.unwrap().name) .collect::>(); From 9d15b041e85d7f30f44e034fdb43003c5917a15b Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Fri, 17 Jun 2022 16:06:44 +0200 Subject: [PATCH 21/28] Remove extra clone, as pointed out by @eladyn --- src/clients/base.rs | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/src/clients/base.rs b/src/clients/base.rs index f7a9fcea..5791bb60 100644 --- a/src/clients/base.rs +++ b/src/clients/base.rs @@ -316,19 +316,13 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-artists-albums) fn artist_albums<'a>( &'a self, - artist_id: ArtistId<'a>, + artist_id: &'a ArtistId<'_>, album_type: Option<&'a AlbumType>, market: Option<&'a Market>, ) -> Paginator<'_, ClientResult> { paginate( move |limit, offset| { - self.artist_albums_manual( - artist_id.clone(), - album_type, - market, - Some(limit), - Some(offset), - ) + self.artist_albums_manual(artist_id, album_type, market, Some(limit), Some(offset)) }, self.get_config().pagination_chunks, ) @@ -337,7 +331,7 @@ where /// The manually paginated version of [`Self::artist_albums`]. async fn artist_albums_manual( &self, - artist_id: ArtistId<'_>, + artist_id: &ArtistId<'_>, album_type: Option<&AlbumType>, market: Option<&Market>, limit: Option, @@ -477,12 +471,10 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-albums-tracks) fn album_track<'a>( &'a self, - album_id: AlbumId<'a>, + album_id: &'a AlbumId<'_>, ) -> Paginator<'_, ClientResult> { paginate( - move |limit, offset| { - self.album_track_manual(album_id.clone(), Some(limit), Some(offset)) - }, + move |limit, offset| self.album_track_manual(album_id, Some(limit), Some(offset)), self.get_config().pagination_chunks, ) } @@ -490,7 +482,7 @@ where /// The manually paginated version of [`Self::album_track`]. async fn album_track_manual( &self, - album_id: AlbumId<'_>, + album_id: &AlbumId<'_>, limit: Option, offset: Option, ) -> ClientResult> { @@ -656,12 +648,12 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-a-shows-episodes) fn get_shows_episodes<'a>( &'a self, - id: ShowId<'a>, + id: &'a ShowId<'_>, market: Option<&'a Market>, ) -> Paginator<'_, ClientResult> { paginate( move |limit, offset| { - self.get_shows_episodes_manual(id.clone(), market, Some(limit), Some(offset)) + self.get_shows_episodes_manual(id, market, Some(limit), Some(offset)) }, self.get_config().pagination_chunks, ) @@ -670,7 +662,7 @@ where /// The manually paginated version of [`Self::get_shows_episodes`]. async fn get_shows_episodes_manual( &self, - id: ShowId<'_>, + id: &ShowId<'_>, market: Option<&Market>, limit: Option, offset: Option, @@ -1080,12 +1072,10 @@ where /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-list-users-playlists) fn user_playlists<'a>( &'a self, - user_id: UserId<'a>, + user_id: &'a UserId<'_>, ) -> Paginator<'_, ClientResult> { paginate( - move |limit, offset| { - self.user_playlists_manual(user_id.clone(), Some(limit), Some(offset)) - }, + move |limit, offset| self.user_playlists_manual(user_id, Some(limit), Some(offset)), self.get_config().pagination_chunks, ) } @@ -1093,7 +1083,7 @@ where /// The manually paginated version of [`Self::user_playlists`]. async fn user_playlists_manual( &self, - user_id: UserId<'_>, + user_id: &UserId<'_>, limit: Option, offset: Option, ) -> ClientResult> { From ccc0d0d2d26758295ce0b460e2b02a5a1f8cdcc9 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Fri, 17 Jun 2022 16:35:44 +0200 Subject: [PATCH 22/28] Fix tests --- tests/test_with_credential.rs | 8 ++++---- tests/test_with_oauth.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_with_credential.rs b/tests/test_with_credential.rs index 6f08b989..8c309a8e 100644 --- a/tests/test_with_credential.rs +++ b/tests/test_with_credential.rs @@ -44,7 +44,7 @@ async fn test_album_tracks() { let birdy_uri = AlbumId::from_uri("spotify:album:6akEvsycLGftJxYudPjmqK").unwrap(); creds_client() .await - .album_track_manual(birdy_uri, Some(2), None) + .album_track_manual(&birdy_uri, Some(2), None) .await .unwrap(); } @@ -71,7 +71,7 @@ async fn test_artists_albums() { creds_client() .await .artist_albums_manual( - birdy_uri, + &birdy_uri, Some(&AlbumType::Album), Some(&Market::Country(Country::UnitedStates)), Some(10), @@ -190,7 +190,7 @@ mod test_pagination { let album = AlbumId::from_uri(ALBUM).unwrap(); let names = client - .album_track(album) + .album_track(&album) .map(|track| track.unwrap().name) .collect::>(); @@ -208,7 +208,7 @@ mod test_pagination { let album = AlbumId::from_uri(ALBUM).unwrap(); let names = client - .album_track(album) + .album_track(&album) .map(|track| track.unwrap().name) .collect::>() .await; diff --git a/tests/test_with_oauth.rs b/tests/test_with_oauth.rs index 16443232..fabd53ad 100644 --- a/tests/test_with_oauth.rs +++ b/tests/test_with_oauth.rs @@ -619,7 +619,7 @@ async fn check_playlist_create(client: &AuthCodeSpotify) -> FullPlaylist { .await .unwrap(); assert_eq!(playlist.id, fetched_playlist.id); - let user_playlists = fetch_all(client.user_playlists(user.id)).await; + let user_playlists = fetch_all(client.user_playlists(&user.id)).await; let current_user_playlists = fetch_all(client.current_user_playlists()).await; assert_eq!(user_playlists.len(), current_user_playlists.len()); From 8689d9451566bbc651fd7c96d88de36cfccf338d Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sat, 2 Jul 2022 13:19:47 +0200 Subject: [PATCH 23/28] Fix typo --- rspotify-model/src/idtypes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 7b4f6213..672305db 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -186,7 +186,7 @@ pub fn parse_uri(uri: &str) -> Result<(Type, &str), IdError> { /// * The `$type` parameter indicates what variant in `Type` the ID is for (say, /// `Artist`, or `Album`). /// * The `$name` parameter is the identifier of the struct. -/// * The `$validity` parameter is the implementation of `is_id_valid`. +/// * The `$validity` parameter is the implementation of `id_is_valid`. macro_rules! define_idtypes { ($($type:ident => { name: $name:ident, From 2796b1e216610785b4bb5ac742dcd85fd27980c3 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sun, 3 Jul 2022 19:07:37 +0200 Subject: [PATCH 24/28] More tests for to_owned --- rspotify-model/src/idtypes.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 672305db..fae2c9c5 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -342,13 +342,13 @@ macro_rules! define_idtypes { /// An ID is a `Cow` after all, so this will switch to the its /// owned version, which has a `'static` lifetime. pub fn into_static(self) -> $name<'static> { - $name(self.0.into_owned().into()) + $name(Cow::Owned(self.0.into_owned())) } /// Similar to [`Self::into_static`], but without consuming the /// original ID. pub fn clone_static(&self) -> $name<'static> { - $name(self.0.clone().into_owned().into()) + $name(Cow::Owned(self.0.clone().into_owned())) } } @@ -632,4 +632,24 @@ mod test { // With owned `Cow` let _ = EpisodeId::from_id(Cow::Owned(ID.to_string())).unwrap(); } + + #[test] + fn test_owned() { + // We check it twice to make sure cloning statically also works. + fn check_static(_: EpisodeId<'static>) {} + + // With lifetime smaller than static because it's a locally owned + // variable. + let local_id = String::from(ID); + + // With `&str`: should be converted + let id: EpisodeId<'_> = EpisodeId::from_id(local_id.as_str()).unwrap(); + check_static(id.clone_static()); + check_static(id.into_static()); + + // With `String`: already static + let id = EpisodeId::from_id(local_id.clone()).unwrap(); + check_static(id.clone()); + check_static(id); + } } From a0b47e614475b02b7d39316c3bcd84534b3cccbc Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sun, 3 Jul 2022 19:13:40 +0200 Subject: [PATCH 25/28] Fix clippy --- src/clients/mod.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/clients/mod.rs b/src/clients/mod.rs index 5a648bbf..84464431 100644 --- a/src/clients/mod.rs +++ b/src/clients/mod.rs @@ -7,6 +7,8 @@ pub use oauth::OAuthClient; use crate::ClientResult; +use std::fmt::Write as _; + use serde::Deserialize; /// Converts a JSON response from Spotify into its model. @@ -17,11 +19,11 @@ pub(in crate) fn convert_result<'a, T: Deserialize<'a>>(input: &'a str) -> Clien /// Append device ID to an API path. pub(in crate) fn append_device_id(path: &str, device_id: Option<&str>) -> String { let mut new_path = path.to_string(); - if let Some(_device_id) = device_id { + if let Some(device_id) = device_id { if path.contains('?') { - new_path.push_str(&format!("&device_id={}", _device_id)); + let _ = write!(new_path, "&device_id={device_id}"); } else { - new_path.push_str(&format!("?device_id={}", _device_id)); + let _ = write!(new_path, "?device_id={device_id}"); } } new_path From 0b63f8943f63968060c3ce034f20f0e371efa8ed Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sun, 3 Jul 2022 19:18:38 +0200 Subject: [PATCH 26/28] Use more string formatting capture --- examples/webapp/src/main.rs | 2 +- rspotify-model/src/idtypes.rs | 4 ++-- src/clients/base.rs | 8 ++++---- src/clients/oauth.rs | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/examples/webapp/src/main.rs b/examples/webapp/src/main.rs index 78d13d0b..b94d3e40 100644 --- a/examples/webapp/src/main.rs +++ b/examples/webapp/src/main.rs @@ -149,7 +149,7 @@ fn index(mut cookies: Cookies) -> AppResponse { AppResponse::Template(Template::render("index", context.clone())) } Err(err) => { - context.insert("err_msg", format!("Failed for {}!", err)); + context.insert("err_msg", format!("Failed for {err}!")); AppResponse::Template(Template::render("error", context)) } } diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index fae2c9c5..29b21640 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -579,7 +579,7 @@ mod test { // Easily testing both ways to obtain an ID test_any(|s| TrackId::from_id_or_uri(s)); test_any(|s| { - let json = format!("\"{}\"", s); + let json = format!("\"{s}\""); serde_json::from_str::<'_, TrackId>(&json) }); } @@ -587,7 +587,7 @@ mod test { /// Serializing should return the Id within it, not the URI. #[test] fn test_serialize() { - let json_expected = format!("\"{}\"", ID); + let json_expected = format!("\"{ID}\""); let track = TrackId::from_uri(URI).unwrap(); let json = serde_json::to_string(&track).unwrap(); assert_eq!(json, json_expected); diff --git a/src/clients/base.rs b/src/clients/base.rs index 301b66f0..392b10b5 100644 --- a/src/clients/base.rs +++ b/src/clients/base.rs @@ -265,7 +265,7 @@ where let ids = join_ids(track_ids); let params = build_map([("market", market.map(|x| x.into()))]); - let url = format!("tracks/?ids={}", ids); + let url = format!("tracks/?ids={ids}"); let result = self.endpoint_get(&url, ¶ms).await?; convert_result::(&result).map(|x| x.tracks) } @@ -293,7 +293,7 @@ where artist_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult> { let ids = join_ids(artist_ids); - let url = format!("artists/?ids={}", ids); + let url = format!("artists/?ids={ids}"); let result = self.endpoint_get(&url, &Query::new()).await?; convert_result::(&result).map(|x| x.artists) @@ -410,7 +410,7 @@ where album_ids: impl IntoIterator> + Send + 'a, ) -> ClientResult> { let ids = join_ids(album_ids); - let url = format!("albums/?ids={}", ids); + let url = format!("albums/?ids={ids}"); let result = self.endpoint_get(&url, &Query::new()).await?; convert_result::(&result).map(|x| x.albums) } @@ -837,7 +837,7 @@ where ("offset", offset.as_deref()), ]); - let url = format!("browse/categories/{}/playlists", category_id); + let url = format!("browse/categories/{category_id}/playlists"); let result = self.endpoint_get(&url, ¶ms).await?; convert_result::(&result).map(|x| x.playlists) } diff --git a/src/clients/oauth.rs b/src/clients/oauth.rs index 60823f4d..44d6e4a0 100644 --- a/src/clients/oauth.rs +++ b/src/clients/oauth.rs @@ -1145,7 +1145,7 @@ pub trait OAuthClient: BaseClient { /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/seek-to-position-in-currently-playing-track) async fn seek_track(&self, position_ms: u32, device_id: Option<&str>) -> ClientResult<()> { let url = append_device_id( - &format!("me/player/seek?position_ms={}", position_ms), + &format!("me/player/seek?position_ms={position_ms}"), device_id, ); self.endpoint_put(&url, &json!({})).await?; @@ -1183,7 +1183,7 @@ pub trait OAuthClient: BaseClient { "volume must be between 0 and 100, inclusive" ); let url = append_device_id( - &format!("me/player/volume?volume_percent={}", volume_percent), + &format!("me/player/volume?volume_percent={volume_percent}"), device_id, ); self.endpoint_put(&url, &json!({})).await?; @@ -1199,7 +1199,7 @@ pub trait OAuthClient: BaseClient { /// /// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/toggle-shuffle-for-users-playback) async fn shuffle(&self, state: bool, device_id: Option<&str>) -> ClientResult<()> { - let url = append_device_id(&format!("me/player/shuffle?state={}", state), device_id); + let url = append_device_id(&format!("me/player/shuffle?state={state}"), device_id); self.endpoint_put(&url, &json!({})).await?; Ok(()) From a75fe4476e4e4193647f28bf7481f6b54c6943a2 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sat, 9 Jul 2022 19:59:00 +0200 Subject: [PATCH 27/28] Update CHANGELOG --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4074675d..9789a708 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ - ([#332](https://github.com/ramsayleung/rspotify/pull/332)) Fix typo in `RestrictionReason` enum values **Breaking changes**: -- ([#305](https://github.com/ramsayleung/rspotify/pull/305)) The `Id` types have been refactored to maximize usability. Instead of focusing on having an object-safe trait and using `dyn`, we now have enums to group up the IDs. This is based on how [`enum_dispatch`](https://docs.rs/enum_dispatch) works, and it's not only easier to use, but also faster. It makes it possible to have borrowed IDs again, so we've chosen to use `Cow` internally for flexibility. Check out the docs for more information! +- ([#305](https://github.com/ramsayleung/rspotify/pull/305)) The `Id` types have been refactored to maximize usability. Instead of focusing on having an object-safe trait and using `dyn Id`, we now have enums to group up the IDs. This is based on how [`enum_dispatch`](https://docs.rs/enum_dispatch) works, and it's not only easier to use, but also more efficient. It makes it possible to have borrowed IDs again, so we've chosen to use `Cow` internally for flexibility. Check out the docs for more information! + + Please let us know if there is anything that could be improved. Unfortunately, this breaks many methods in `BaseClient` and `OAuthClient`, but the errors should occur at compile-time only. - ([#325](https://github.com/ramsayleung/rspotify/pull/325)) The `auth_code`, `auth_code_pkce`, `client_creds`, `clients::base` and `clients::oauth` modules have been removed from the public API; you should access the same types from their parent modules instead - ([#326](https://github.com/ramsayleung/rspotify/pull/326)) The `rspotify::clients::mutex` module has been renamed to `rspotify::sync` - ([#330](https://github.com/ramsayleung/rspotify/pull/330)) `search` now accepts `Option` instead of `Option<&IncludeExternal>` From db0d7facb0b943f9c6564895506af6624b11b123 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Sat, 9 Jul 2022 20:13:27 +0200 Subject: [PATCH 28/28] Minor improvements --- rspotify-model/src/idtypes.rs | 43 ++++++++++++++++++++++++++++++----- rspotify-model/src/lib.rs | 1 + src/lib.rs | 12 +--------- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/rspotify-model/src/idtypes.rs b/rspotify-model/src/idtypes.rs index 29b21640..b44c320a 100644 --- a/rspotify-model/src/idtypes.rs +++ b/rspotify-model/src/idtypes.rs @@ -126,7 +126,7 @@ pub enum IdError { /// /// [module level documentation]: [`crate::idtypes`] #[enum_dispatch] -pub trait Id<'a> { +pub trait Id { /// Returns the inner Spotify object ID, which is guaranteed to be valid for /// its type. fn id(&self) -> &str; @@ -324,7 +324,6 @@ macro_rules! define_idtypes { /// long enough when using `Into>`, so the only /// sensible choice is to just use a `&str`. pub fn from_id_or_uri(id_or_uri: &'a str) -> Result { - let id_or_uri = id_or_uri.into(); match Self::from_uri(id_or_uri) { Ok(id) => Ok(id), Err(IdError::InvalidPrefix) => Self::from_id(id_or_uri), @@ -335,7 +334,7 @@ macro_rules! define_idtypes { /// This creates an ID with the underlying `&str` variant from a /// reference. Useful to use an ID multiple times without having /// to clone it. - pub fn as_ref(&'a self) -> $name<'a> { + pub fn as_ref(&'a self) -> Self { Self(Cow::Borrowed(self.0.as_ref())) } @@ -352,7 +351,7 @@ macro_rules! define_idtypes { } } - impl<'a> Id<'a> for $name<'a> { + impl Id for $name<'_> { fn id(&self) -> &str { &self.0 } @@ -484,7 +483,7 @@ pub enum PlayContextId<'a> { } // These don't work with `enum_dispatch`, unfortunately. impl<'a> PlayContextId<'a> { - pub fn as_ref(&'a self) -> PlayContextId<'a> { + pub fn as_ref(&'a self) -> Self { match self { PlayContextId::Artist(x) => PlayContextId::Artist(x.as_ref()), PlayContextId::Album(x) => PlayContextId::Album(x.as_ref()), @@ -492,6 +491,24 @@ impl<'a> PlayContextId<'a> { PlayContextId::Show(x) => PlayContextId::Show(x.as_ref()), } } + + pub fn into_static(self) -> PlayContextId<'static> { + match self { + PlayContextId::Artist(x) => PlayContextId::Artist(x.into_static()), + PlayContextId::Album(x) => PlayContextId::Album(x.into_static()), + PlayContextId::Playlist(x) => PlayContextId::Playlist(x.into_static()), + PlayContextId::Show(x) => PlayContextId::Show(x.into_static()), + } + } + + pub fn clone_static(&'a self) -> PlayContextId<'static> { + match self { + PlayContextId::Artist(x) => PlayContextId::Artist(x.clone_static()), + PlayContextId::Album(x) => PlayContextId::Album(x.clone_static()), + PlayContextId::Playlist(x) => PlayContextId::Playlist(x.clone_static()), + PlayContextId::Show(x) => PlayContextId::Show(x.clone_static()), + } + } } /// Grouping up multiple kinds of IDs to treat them generically. This also @@ -503,12 +520,26 @@ pub enum PlayableId<'a> { } // These don't work with `enum_dispatch`, unfortunately. impl<'a> PlayableId<'a> { - pub fn as_ref(&'a self) -> PlayableId<'a> { + pub fn as_ref(&'a self) -> Self { match self { PlayableId::Track(x) => PlayableId::Track(x.as_ref()), PlayableId::Episode(x) => PlayableId::Episode(x.as_ref()), } } + + pub fn into_static(self) -> PlayableId<'static> { + match self { + PlayableId::Track(x) => PlayableId::Track(x.into_static()), + PlayableId::Episode(x) => PlayableId::Episode(x.into_static()), + } + } + + pub fn clone_static(&'a self) -> PlayableId<'static> { + match self { + PlayableId::Track(x) => PlayableId::Track(x.clone_static()), + PlayableId::Episode(x) => PlayableId::Episode(x.clone_static()), + } + } } #[cfg(test)] diff --git a/rspotify-model/src/lib.rs b/rspotify-model/src/lib.rs index 6c80573b..d0b1242d 100644 --- a/rspotify-model/src/lib.rs +++ b/rspotify-model/src/lib.rs @@ -53,6 +53,7 @@ impl PlayableItem { /// /// Note that if it's a track and if it's local, it may not have an ID, in /// which case this function will return `None`. + #[must_use] pub fn id(&self) -> Option> { match self { PlayableItem::Track(t) => t.id.as_ref().map(|t| PlayableId::Track(t.as_ref())), diff --git a/src/lib.rs b/src/lib.rs index aae6cac0..2c1c9f0e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -286,18 +286,8 @@ pub(in crate) fn generate_random_string(length: usize, alphabet: &[u8]) -> Strin .collect() } -// TODO: is it possible to take a `IntoIterator` instead of a -// `IntoIterator`? Now we have `Id<'a>` instead of `&'a T`, so -// technically `IntoIterator` is the same as `IntoIterator>`. Otherwise, this function is taking a double reference, and requires -// more boilerplate. -// -// However, this seems to be impossible because then the function would own the -// Ids, and then we can't call `Id::id`. -// -// Hack: turning this into a macro (best if avoided). #[inline] -pub(in crate) fn join_ids<'a, T: Id<'a> + 'a>(ids: impl IntoIterator) -> String { +pub(in crate) fn join_ids<'a, T: Id + 'a>(ids: impl IntoIterator) -> String { let ids = ids.into_iter().collect::>(); ids.iter().map(Id::id).collect::>().join(",") }