-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Keep polishing the models #157
Conversation
I came up with a different solution to remove repetitive work from the endpoints, only needing trait Extract {
type InnerT;
fn extract(self) -> Self::InnerT;
}
pub(in crate) struct BaseModel {
onlyfield: Vec<String>
}
impl Extract for BaseModel {
type InnerT = Vec<String>;
fn extract(self) -> Self::InnerT {
self.onlyfield
}
} It's not that much of a change, but at least I would make the simplified model public but only in crate. I don't like declaring it inside the endpoint. The |
In fact, the trait Extract {
type InnerT;
fn extract(self) -> Self::InnerT;
}
macro_rules! simplified_model {
($vis:vis struct $name:ident { $attr_name:ident : $attr_type:ty }) => {
#[derive(Debug)] // You can add any repetitive derives here
$vis struct $name {
$attr_name: $attr_type
}
impl Extract for $name {
type InnerT = $attr_type;
fn extract(self) -> Self::InnerT {
self.$attr_name
}
}
}
}
simplified_model! {
pub(in crate) struct BaseModel {
onlyfield: Vec<String>
}
} ...which I've tested and works, but it's up to you if you consider it necessary as well. If you need different I'm aware the trait is a bit dumb, we could just name all the inner members the same - say, |
Yes, I have to confess that declaring model inside the endpoints is not a good idea, since if multiple endpoints return a same wrapper object, we will define it repeatedly inside different endpoints. On the other side, I think define a new trait and macro to extra the valuable field from wrapper model is over-design, because wrapper objects mostly are a simple struct, it's easy to extract field with So I personally prefer to redefine those wrapper objects with |
@marioortizmanero I think this PR is ready to review, as for this TODO item:
I have created an issue #163 to discuss that is there a better choice to implement it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I haven't found any issues other than small grammatical nitpicks. Really great work :)
src/model/mod.rs
Outdated
} | ||
} | ||
|
||
/// Deserialize `std::time::Duration` from millisecond(represented as u64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments could have an extra space before the parenthesis. millisecond
could be changed to milliseconds
as well.
src/model/mod.rs
Outdated
d.deserialize_u64(DateTimeVisitor) | ||
} | ||
|
||
/// Serialize DateTime<Utc> to Unix millisecond timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's two spaces between DateTime<Utc>
and to
I would fix those problems later before merging :) |
Description
Keep polish the models by following the issues:
Motivation and Context
To make
rspotify
's API easier to use.FullTracks
CursorPageFullArtists
SeversalSimplifiedShows
DevicePayload
AudioFeaturesPayload
PageCategory
PageSimpliedAlbums
FullAlbums
FullArtists
AudioAnalysisSection::mode
andAudioFeatures::mode
aref32s
but should beOption<Mode>s
whereenum Mode { Major, Minor }
as it is more useful.CursorBasedPage
should also not include the URLs.AudioFeatures.duration_ms
to duration, and change its type fromu32
tostd::time::Duration
.FullEpisode.duration_ms
to duration, and change its type fromu32
tostd::time::Duration
.SimplifiedEpisode.duration_ms
to duration, and change its type fromu32
tostd::time::Duration
.FullTrack.duration_ms
to duration, and change its type fromu32
tostd::time::Duration
.SimplifiedTrack.duration_ms
to duration, and change its type fromu32
tostd::time::Duration
.SimplifiedEpisode::duration_ms
should be aDuration
.Offset::position
should be aDuration
CurrentlyPlayingContext::progress_ms
should beprogress
:Option<Duration>
.CurrentPlaybackContext::progress_ms
should beprogress
:Option<Duration>
.ResumePoint::resume_position_ms
should be aDuration
.CurrentlyPlayingContext::timestamp
should be aDateTime<Utc>
CurrentPlaybackContext::timestamp
should be aDateTime<Utc>
(SimplifiedPlayingContext::timestamp
should be aDateTime<Utc>
SimplifiedPlayingContext
is useless, just remove it.)Dependencies
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
FullArtists
:test_artist_related_artists
,test_artists
FullTracks
:test_artist_top_tracks
,test_tracks
CursorPageFullArtists
:test_current_user_followed_artists
SeversalSimplifiedShows
:test_get_seversal_shows
DevicePayload
:test_device
AudioFeaturesPayload
:test_audios_features
PageCategory
:test_categories
PageSimpliedAlbums
:test_new_releases
FullAlbums
:test_albums
duration_ms
toduration
, and change its type fromf32
tostd::time::Duration
AudioFeatures
:test_audio_features
FullEpisode
:test_full_episode
SimplifiedEpisode
:simplified_episode
FullTrack
:test_full_track
SimplifiedTrack
:test_simplified_track
ResumePoint::resume_position_ms
should be aDuration
:test_resume_point
CurrentlyPlayingContext/CurrentlyPlaybackContext/SimplifiedPlayingContext::timestamp
should be aDateTime<Utc>
CurrentlyPlayingContext
:test_currently_playing_context
CurrentPlaybackContext
:test_current_playback_context
Offset::position
should be aDuration
:test_offset
PS: it seems the pull request template doesn't works, I just add the content manually, and I would like to open a new PR to test the pull request template individually