Meris/refactor playable playcontext #305
Conversation
|
There's a lot of more doc changes required for this, but I wanted to make sure that the code compiles before changing it. WDYT? |
|
That is another very nice approach to IDs. It would certainly simplify some parts of their implementation, and we could go back to having IDs with lifetimes. But would you really say that using enums is easier than dynamic dispatch? As far as I know, the code you included would be basically the same: let sliced = track_ids
.into_iter()
.map(|track_id| TrackId::from_id(&track_id).unwrap())
.collect::<Vec<_>>();
let playable = sliced
.iter()
.map(Playable::Track)
.collect::<Vec<_>>();The main benefit would be that even after constructing |
Thanks for the reply! Just to be clear: I'm really a noob at rust, I don't know what I'm doing. But what I noticed is that I could (according to me) do it in 1 iteration like this: let playable = track_ids
.into_iter()
.map(|track_id| Playable::Track(TrackId::from_id(&track_id).unwrap()))
.collect::<Vec<_>>();But I couldn't figure out how to do this with the I was able to finish my little recreational project anyways, so it's fine if we keep it the way it is! |
|
You're right. It's not possible to do that in one call in the case of @ramsayleung, what do you think we go back to the original |
|
It feels like a century past after @marioortizmanero refactored the The origination of this refactoring is that we have a problem with lists of Ids don't accept multiple of them #203: let uris = &[
EpisodeId::from_uri("spotify:episode:7zC6yTmY67f32WOShs0qbb").unwrap(),
EpisodeId::from_uri("spotify:episode:3hCrwz19QlXY0myYh0XgmL").unwrap(),
TrackId::from_uri("spotify:track:0hAMkY2kwdXPPDfQ1e3BmJ").unwrap()
];Then Mario created 4 proposals to fix this problem:
I personally suggest to @merisbahti to take a look at these proposals, and what are the advantages and disadvantages of the current solution compared with those proposals? Can this approach solve the original problem #203 without introducing some new issues? |
|
Damn, looks like you've put a lot of thought into this! Fun! I'll take a look, though I'm not fast to reply hehe. |
|
To be fair it's going to be a lot of reading, I did multiple iterations as I was trying to understand the original problem. The issue I'm seeing with the enum is that you lose the ability to call methods in the match id {
PlayableId::Track(id) => id.uri(),
PlayableId::Episode(id) => id.uri(),
// ...
}This part could be implemented with a crate such as: https://docs.rs/enum_dispatch/latest/enum_dispatch/. I'm a bit hesitant to adding new libraries, but thanks to it we may be able to make IDs more:
We'd have to consider if the change is worth it. Please let me know if there's anything I missed. |
marioortizmanero
force-pushed
the
meris/refactor-playable-playcontext
branch
from
dc893b1
to
1fce826
Compare
|
I'm now free from finals and ready to work on this for a bit. I am modifying @merisbahti's PR to clean up the |
|
Alright, first iteration done. Please take a look when you have some time, @ramsayleung, and let me know what you think. Now that we don't need object safety, we can go back to the unsized The main issue I found is that it's illegal to have unsized enums, so for the I know it might be a bit hard to understand, especially because of the heavy usage of macros, which I would like to reduce. I have to update the docs as well, so don't look at them too much. If you have any questions don't hesitate to ask them, and please point out any possible simplifications you may find. |
|
Thanks for pinging me and working on this. The current version would definitely simplify the code from #218 a lot. dyns and Boxes are now gone! That is, how it'd look like.b.method("OpenUri", ("uri",), (), move |_, _, (uri,): (String,)| {
enum Uri<'a> {
Playable(PlayableId<'a>),
Context(PlayContextId<'a>),
}
impl Uri<'_> {
fn from_id<'a>(id_type: Type, id: &'a str) -> Result<Uri<'a>, IdError> {
use Uri::*;
let uri = match id_type {
Type::Track => Playable(PlayableId::Track(TrackId::from_id(id)?)),
Type::Episode => Playable(PlayableId::Episode(EpisodeId::from_id(id)?)),
Type::Artist => Context(PlayContextId::Artist(ArtistId::from_id(id)?)),
Type::Album => Context(PlayContextId::Album(AlbumId::from_id(id)?)),
Type::Playlist => Context(PlayContextId::Playlist(PlaylistId::from_id(id)?)),
Type::Show => Context(PlayContextId::Show(ShowId::from_id(id)?)),
Type::User | Type::Collection => return Err(IdError::InvalidType),
};
Ok(uri)
}
}
let mut chars = uri
.strip_prefix("spotify")
.ok_or_else(|| MethodErr::invalid_arg(&uri))?
.chars();
let sep = match chars.next() {
Some(ch) if ch == '/' || ch == ':' => ch,
_ => return Err(MethodErr::invalid_arg(&uri)),
};
let rest = chars.as_str();
let (id_type, id) = rest
.rsplit_once(sep)
.and_then(|(id_type, id)| Some((id_type.parse::<Type>().ok()?, id)))
.ok_or_else(|| MethodErr::invalid_arg(&uri))?;
let uri = Uri::from_id(id_type, id).map_err(|_| MethodErr::invalid_arg(&uri))?;
let device_name = utf8_percent_encode(&mv_device_name, NON_ALPHANUMERIC).to_string();
let device_id = sp_client.device().ok().and_then(|devices| {
devices.into_iter().find_map(|d| {
if d.is_active && d.name == device_name {
d.id
} else {
None
}
})
});
match uri {
Uri::Playable(id) => {
let _ = sp_client.start_uris_playback(
Some(id),
device_id.as_deref(),
Some(Offset::for_position(0)),
None,
);
}
Uri::Context(id) => {
let _ = sp_client.start_context_playback(
id,
device_id.as_deref(),
Some(Offset::for_position(0)),
None,
);
}
}
Ok(())
});Regarding the different owned (e.g. pub struct ArtistId<'a>(Cow<'a, str>);
pub struct TrackId<'a>(Cow<'a, str>);
pub struct PlaylistId<'a>(Cow<'a, str>);
pub struct AlbumId<'a>(Cow<'a, str>);
pub struct EpisodeId<'a>(Cow<'a, str>);
pub struct UserId<'a>(Cow<'a, str>);Those types would then contain both the owned and the unowned variant. As such, some boilerplate code could probably be removed. I played a bit with that idea, and it seems like it could work out. You can see a very raw (and not compiling) prototype here. Some things that I noticed:
See also this article, which covers a similar use case. Looking forward to hearing your thoughts on this! |
|
That does look a bit better. What I really don't like is that you have to implement the parsing part yourself. I guess we could have an About the |
Yeah, that sounds reasonable. Something that might make sense as well, are methods on that fn as_playable_id(&self) -> Option<PlayableId>;
fn to_playable_id(self) -> Option<PlayableIdBuf>;
fn as_play_context_id(&self) -> Option<PlayContextId>;
fn to_play_context_id(self) -> Option<PlayContextIdBuf>;But those might also be a bit specific and easy enough to implement that it seems acceptable to leave matching An alternative, which is a bit hacky and inefficient however, would be, to offer the |
Definitely, we can add those for usability. Using
I would definitely leave it like this, check out the last commit. I still have to fix a couple things but it's late already so I've left it as TODOs and I'll try to fix them when I can (any help appreciated). Thanks a lot for your feedback :) |
marioortizmanero
force-pushed
the
meris/refactor-playable-playcontext
branch
from
a08da06
to
a46a922
Compare
|
That is splendid, thanks a lot for the help with your branch. Sorry for the crappy code, it was late in the night :P I'll merge it here and do one last review before we're done. |
|
The only thing that's bothering me is the double reference in cases like: fn playlist_items<'a>(
&'a self,
playlist_id: &'a PlaylistId<'_>,
fields: Option<&'a str>,
market: Option<&'a Market>,
) -> Paginator<'_, ClientResult<PlaylistItem>> {
paginate(
move |limit, offset| {
self.playlist_items_manual(
playlist_id.as_borrowed(),
fields,
market,
Some(limit),But I think there's nothing we can do. All the new appearances of Also, do you think you could spin up a quick benchmark for both approaches to ID joining before we use the more complex one? You could use the one I did in the past as a reference: #161 (comment) |
Well, it's only a double reference, if the underlying variant is the
Yeah, sure, name it however you want. I was initially going to ask you about the method name, but that somehow got lost somewhere in the process. I would personally opt for
Yeah, I'll happily experiment with that, probably tomorrow, however. |
|
I've finally got around to benchmarking the different approaches. As you will see, I also threw a third implementation in there, which makes use of the unstable #![feature(test, iter_intersperse)]
extern crate test;
fn main() {}
trait Id<'a> {
fn id(&self) -> &str;
}
impl Id<'_> for &str {
fn id(&self) -> &str {
self
}
}
#[inline]
pub(in crate) fn join_ids<'a, T: Id<'a> + 'a>(ids: impl IntoIterator<Item = T>) -> 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
}
#[inline]
pub(in crate) fn join_ids_collect<'a, T: Id<'a> + 'a>(ids: impl IntoIterator<Item = T>) -> String {
let ids = ids.into_iter().collect::<Vec<_>>();
ids.iter().map(Id::id).collect::<Vec<_>>().join(",")
}
#[inline]
pub(in crate) fn join_ids_to_string<'a, T: Id<'a> + 'a>(ids: impl IntoIterator<Item = T>) -> String {
ids.into_iter().map(|id| id.id().to_string()).collect::<Vec<_>>().join(",")
}
#[inline]
pub(in crate) fn join_ids_intersperse<'a, T: Id<'a> + 'a>(ids: impl IntoIterator<Item = T>) -> String {
let ids = ids.into_iter().collect::<Vec<_>>();
ids.iter().map(Id::id).intersperse(",").collect()
}
#[cfg(test)]
mod tests {
use test::Bencher;
const MAX: u32 = 100_000;
fn initial_vec() -> Vec<&'static str> {
let mut v = Vec::new();
for i in 1..=MAX {
if i % 2 == 0 {
v.push("even number here");
} else {
v.push("odd number over here");
}
}
v
}
#[bench]
fn bench_custom_join(b: &mut Bencher) {
let data = initial_vec();
b.iter(|| {
super::join_ids(data.iter().copied());
});
}
#[bench]
fn bench_join_intersperse(b: &mut Bencher) {
let data = initial_vec();
b.iter(|| {
super::join_ids_intersperse(data.iter().copied());
});
}
#[bench]
fn bench_join_double_collect(b: &mut Bencher) {
let data = initial_vec();
b.iter(|| {
super::join_ids_collect(data.iter().copied());
});
}
#[bench]
fn bench_join_to_string(b: &mut Bencher) {
let data = initial_vec();
b.iter(|| {
super::join_ids_to_string(data.iter().copied());
});
}
}The results are … interesting, and I'll probably leave a proper interpretation and conclusion up to you:
While the method involving Now, between the "custom" joining and the Interested in reading your thoughts on that one! |
|
Agreed, let's rename it to Regarding joining the Ids, yeah, we have to remember that the size of the iterator won't actually be that large most times. Only after #298 you can actually get past the API limits, but even then I'm not sure if someone would ever pass 100.000 songs. I would personally do the double collect just because it's more maintainable. It seems more efficient for smaller sizes, but that's less importantly since it's quite approximate. Pretty cool that you tried |
|
Oh, I just realized that clippy prints out this warning: Maybe it would be more efficient with its suggestion? Would you mind re-running the benchmarks with that real quick before I change it? Not sure if it works like that for |
|
I've modified the groups so that they're implemented automatically with the macro
Some more questions:
|
Oh, that's interesting. Unfortunately, it seems like this'd be only a cosmetic change, as it doesn't change the results much, from what I can see. So I would agree with you going for the double collect.
Yeah, that definitely looks much cleaner!
With that and the
I will add them among my other few last comments in the review below. The only “bigger” thing that came up when experimenting with the new API is the following: As far as I can tell, there is currently no easy way to directly construct an owned variant of any |
|
Ok, I've fixed everything, even some examples that we were missing. I've also added string interpolation where I could, as I'll merge #294 after this. I've opened #324 for the
Yeah that's a good point. We could add something like |
| /// | ||
| /// 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<S>(id: S) -> Self |
There was a problem hiding this comment.
I am not sure if it's necessary to use unsafe statement?
There was a problem hiding this comment.
I mean, we aren't technically violating memory safety here, but we are violating the integrity of the ID type system. So I thought unsafe was a good enough indicator to be careful about its usage. I guess it's weird because we don't actually have unsafe code inside it, so we could use a crate like https://docs.rs/unsafe_fn/latest/unsafe_fn/. I would use this new lint, however: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/builtin/static.UNSAFE_OP_IN_UNSAFE_FN.html
What do you think?
There was a problem hiding this comment.
I think it should be unsafe as we are really violating type safety in the sense we violate guarantees of id types here. So it's up to programmer to uphold these guarantees, hence it's unsafe.
rspotify-model/src/idtypes.rs
Outdated
| } | ||
| // These don't work with `enum_dispatch`, unfortunately. | ||
| impl<'a> PlayContextId<'a> { | ||
| pub fn as_ref(&'a self) -> PlayContextId<'a> { |
There was a problem hiding this comment.
I am a little confused about how this as_ref function works?
There was a problem hiding this comment.
It makes it possible to reborrow the internal string so that no cloning is necessary. It just takes another immutable reference to the inner contents, with the same lifetime.
This is what I'm the most unhappy about in the PR. It is somewhat annoying to use but I think it's the only way.
|
If I don't misunderstand the insight of this PR, the whole design of the PR would look like that:
The key problem is that some endpoints accept So we need dynamic dispatch mechanism, our previous implementation was built on This is my concern too, in order to know the
The source code of the `plantuml` diagram:@startuml
abstract Id{
+{abstract}id(&self) -> &str
+{abstract}_type(&self) -> Type
==
+uri(&self) -> String
+url(&self) -> String
+parse_uri(uri: &str) -> Result<(Type, &str), IdError>
}
note left of Id::uri
The implemetation of uri is `spotify:{type}:{id}`
end note
abstract PlayContextId extends Id
abstract PlayableId extends Id
note right of AlbumId::_type
The `Type` of AlbumId is `Album`
end note
PlayContextId <|-- AlbumId
class AlbumId{
[type = Album]
==
Cow<'a, str> id_data
==
+ id(&self) -> &str
+ _type(&self) -> Type
==
+id_is_valid(id: &str) -> bool
+from_id_unchecked(id: S) -> Self
+from_id(id: S) -> Result<Self, IdError>
+from_uri(uri: &'a str) -> Result<Self, IdError>
+from_id_or_uri(id_or_uri: &'a str) -> Result<Self, IdError>
+as_ref(&'a self) -> AlbumId<'a>
+into_static(self) -> AlbumId<'static>
+clone_static(&self) -> AlbumId<'static>
}
note bottom of TrackId
Other Ids are mostly similar to AlbumId, so just dismiss their details
end note
PlayContextId <|-- TrackId
class TrackId{
[type = Track]
...
}
PlayContextId <|-- PlaylistId
class PlaylistId{
[type = Playlist]
...
}
PlayContextId <|-- ArtistId
class ArtistId{
[type = Artist]
...
}
PlayableId <|-- ShowId
class ShowId{
[type = Show]
...
}
PlayableId <|-- EpisodeId
class EpisodeId{
[type = Episode]
...
}
Id <|-- UserId
class UserId{
[type = User]
...
}
abstract OAuthClient{
+ start_context_playback(&self, context_uri: <b>PlayContextId</b><'_>) -> ClientResult<()>
+ start_uris_playback<'a>(&self, uris: impl IntoIterator<Item = <b>PlayableId</b><'a>>) -> ClientResult<()>
+ playlist_add_items( &self, items: impl IntoIterator<Item = <b>PlayableId</b><'a>>) -> ClientResult<PlaylistResult>
+ playlist_replace_items<'a>(&self, items: impl IntoIterator<Item = <b>PlayableId</b><'a>>) -> ClientResult<()>
+ playlist_remove_all_occurrences_of_items( &self, track_ids: impl IntoIterator<Item = <b>PlayableId</b><'a>>) -> ClientResult<PlaylistResult>
+ start_uris_playback<'a>( &self, uris: impl IntoIterator<Item = <b>PlayableId</b><'a>>) -> ClientResult<()>
+ add_item_to_queue( &self, item: <b>PlayableId</b><'_>,) -> ClientResult<()>
}
@enduml |
|
Great job on the diagram! It's quite well made, but it doesn't feel right because we aren't really using inheritance anymore. As in,
I think we don't need the |
Yes, I know that, but it's a little hard to express
Cool! The source code of the `plantuml` diagram:@startuml
abstract Id{
+{abstract}id(&self) -> &str
+{abstract}_type(&self) -> Type
==
+uri(&self) -> String
+url(&self) -> String
+parse_uri(uri: &str) -> Result<(Type, &str), IdError>
}
note left of Id::uri
The implemetation of uri is `spotify:{type}:{id}`
end note
enum PlayContextId implements Id
enum PlayContextId{
AlbumId
TrackId
PlaylistId
ArtistId
}
enum PlayableId implements Id
enum PlayableId{
ShowId
EpisodeId
}
note bottom of PlayContextId
call dynamic-dispatched methods by `enum_dispatch` crate with almost no overhead.
end note
note right of AlbumId::_type
The `Type` of AlbumId is `Album`
end note
Id <|-- AlbumId
class AlbumId{
[type = Album]
==
Cow<'a, str> id_data
==
+ id(&self) -> &str
+ _type(&self) -> Type
==
+id_is_valid(id: &str) -> bool
+from_id_unchecked(id: S) -> Self
+from_id(id: S) -> Result<Self, IdError>
+from_uri(uri: &'a str) -> Result<Self, IdError>
+from_id_or_uri(id_or_uri: &'a str) -> Result<Self, IdError>
+as_ref(&'a self) -> AlbumId<'a>
+into_static(self) -> AlbumId<'static>
+clone_static(&self) -> AlbumId<'static>
}
note bottom of TrackId
Other Ids are mostly similar to AlbumId, so just dismiss their details
end note
Id <|-- TrackId
class TrackId{
[type = Track]
...
}
Id <|-- PlaylistId
class PlaylistId{
[type = Playlist]
...
}
Id <|-- ArtistId
class ArtistId{
[type = Artist]
...
}
Id <|-- ShowId
class ShowId{
[type = Show]
...
}
Id <|-- EpisodeId
class EpisodeId{
[type = Episode]
...
}
Id <|-- UserId
class UserId{
[type = User]
...
}
abstract OAuthClient{
+ start_context_playback(&self, context_uri: <b>PlayContextId</b><'_>) -> ClientResult<()>
+ start_uris_playback<'a>(&self, uris: impl IntoIterator<Item = <b>PlayableId</b><'a>>) -> ClientResult<()>
+ playlist_add_items( &self, items: impl IntoIterator<Item = <b>PlayableId</b><'a>>) -> ClientResult<PlaylistResult>
+ playlist_replace_items<'a>(&self, items: impl IntoIterator<Item = <b>PlayableId</b><'a>>) -> ClientResult<()>
+ playlist_remove_all_occurrences_of_items( &self, track_ids: impl IntoIterator<Item = <b>PlayableId</b><'a>>) -> ClientResult<PlaylistResult>
+ start_uris_playback<'a>( &self, uris: impl IntoIterator<Item = <b>PlayableId</b><'a>>) -> ClientResult<()>
+ add_item_to_queue( &self, item: <b>PlayableId</b><'_>,) -> ClientResult<()>
}
@enduml |
|
Cool. Just one more thing before adding that image to the docs: it seems like the I've just added one more test for the |
I think it's not duplicate, one of them is
I think we could include it inside the
oooh, Good catch.
Probably, we could add the list of endpoints we just break in the CHANGELOG, for example |
Sorry, not quite sure what you meant here, @ramsayleung. It's useful to have it as a separate function so that anyone can implement their own
I honestly don't think it's worth listing the broken methods. I was doing it but literally 90% are broken and it's just noise in the changelog, I would say. Shall we merge like this? Thanks for the last review, @eladyn, I've just fixed your suggestions. I've also:
The |
marioortizmanero
force-pushed
the
meris/refactor-playable-playcontext
branch
from
a73383b
to
e182979
Compare
marioortizmanero
force-pushed
the
meris/refactor-playable-playcontext
branch
from
e182979
to
db0d7fa
Compare
|
Coming back to what you wrote in #305 (comment): I finally managed to get it (kind of) working the way I imagined it could work. There might be a simpler way, but the way I solved this is accepting a generic context to be passed into a All in all, it's a bit ugly, but none of that ugliness reaches “user code”, so it might be fine? You can find the code here. In order to not clutter this PR even more, do you think it would be a good idea to open a new one after this is merged? We could then discuss those changes separately. |
It makes sense :)
I get your point :) |
| @@ -314,7 +314,7 @@ 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: &'a ArtistId<'_>, | |||
There was a problem hiding this comment.
artist_id: &'a ArtistId<'_>, should be artist_id: ArtistId<'_>,? Because we don't need the explicit life notation 'a.
There was a problem hiding this comment.
I'm going to reply here, but this applies to several other review comments too (every auto-paginated endpoint, not those with _manual):
This is something that we discussed earlier on this PR, see these comments and this one. Omitting the & reference works in the sync case, but not in the async one (because the closure passed to paginate would return a Future that contains a reference to the id that is owned by the closure).
As I wrote here, I found a potential solution to this problem, which can be found in eladyn@b2bfaff. That solution is a bit more complex however and might be better suited for a follow-up PR?
| @@ -7,6 +7,8 @@ pub use oauth::OAuthClient; | |||
|
|
|||
| use crate::ClientResult; | |||
|
|
|||
| use std::fmt::Write as _; | |||
There was a problem hiding this comment.
I am a little confused, what does this statement mean?
There was a problem hiding this comment.
It just imports the trait without getting it into the namespace. We just want to be able to use the methods in the trait, but not really Write itself, as it may collide or get in the way.
| ids.into_iter().map(Id::id).collect::<Vec<_>>().join(",") | ||
| pub(in crate) fn join_ids<'a, T: Id + 'a>(ids: impl IntoIterator<Item = T>) -> String { | ||
| let ids = ids.into_iter().collect::<Vec<_>>(); | ||
| ids.iter().map(Id::id).collect::<Vec<_>>().join(",") |
There was a problem hiding this comment.
Do we need to collect twice to join_ids, it will allocate resource twice?
There was a problem hiding this comment.
Please refer to this: #305 (comment)
|
Great, I think we are ready to merge this PR? |
|
Sure, merging this! Thanks for the proposed fix, @eladyn, I'll open a new PR for that and we can discuss it before the new version. |
Description
Basically change the two traits
PlayContextIdandPlayableId, from this:To use enums, like this:
Motivation and Context
So that we don't have to use dynamic dispatch, and don't have to see code like this:
Dependencies
None
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration
Is this change properly documented?
Please make sure you've properly documented the changes you're making.
Don't forget to add an entry to the CHANGELOG if necessary (new features, breaking changes, relevant internal improvements).