Skip to content
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

Polish Rspotify #128

Merged
merged 48 commits into from
Oct 27, 2020
Merged

Polish Rspotify #128

merged 48 commits into from
Oct 27, 2020

Conversation

ramsayleung
Copy link
Owner

@ramsayleung ramsayleung commented Sep 19, 2020

Clean up Rspotify, take some points of #127 into consideration.

  • remove convert_map_to_string and convert_string_to_map in util.rs, replaced them with Reqwest's query method and Url struct.
  • Many functions like Spotify::tracks take in a Vec<&str>/Vec/&[String]/&[&str], when they should take in an impl IntoIterator<Item = &str>.
  • Split single senum.rs file into a module named enums and move it into model module.
  • reexport struct in model module to allow users to write rspotify::model::FullAlbum instead of the overly verbose rspotify::model::album::FullAlbum.
  • Rename CurrentlyPlaybackContext to CurrentPlaybackContext.
  • Change release_date_precision to enum.

@ramsayleung ramsayleung changed the title Clean up Rspotify Polish Rspotify Sep 19, 2020
@ramsayleung ramsayleung mentioned this pull request Sep 20, 2020
8 tasks
CHANGELOG.md Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/oauth2.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ramsayleung ramsayleung mentioned this pull request Sep 22, 2020
87 tasks
@ramsayleung
Copy link
Owner Author

ramsayleung commented Sep 22, 2020

@marioortizmanero I think this PR is ready to review, as for the rest of issues mention in #127 which are mostly about Model, I prefer to create a new PR, otherwise this PR will grow up to be a huge one which is difficult to review.

@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Sep 22, 2020

The thing is that I'm doing tons of changes at #129, specially related to the endpoints, so the merge conflict is going to be quite big if we use this right now. #129 is almost ready as well, can you start taking a look in there? If you can help me to update the docs and tests it'd be super useful.

@ramsayleung
Copy link
Owner Author

#129 is almost ready as well, can you start taking a look in there? If you can help me to update the docs and tests it'd be super useful.

Yeah, Sure. I am gonna to review thie PR, and collaborate to update docs and tests :)

@ramsayleung
Copy link
Owner Author

ramsayleung commented Oct 23, 2020

I have refactored enums module with strum crate, reduced line-of-code from 7396 to 6972, generated by tokei. It's much clear and readable than before, but we have introduced a new dependency, I think about that will it increase the compilation time ? The number of compilation units increased from 159 to 163 with default feature.

@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Oct 23, 2020

That's great! Have you measured the compilation times, rather than the number of units? IMO this dependency is worth it because it just removes so much boilerplate, and boilerplate is prone to bugs.

@ramsayleung
Copy link
Owner Author

ramsayleung commented Oct 24, 2020

Yes, I have measured the compilation times in my laptop with Intel i5-4200M:

git checkout 72c6ee3 # the last commit without `strum` crate.
> hyperfine --warmup 2 'cargo clean && cargo build --no-default-features --features client-reqwest' -i
Benchmark #1: cargo clean && cargo build --no-default-features --features client-reqwest
  Time (mean ± σ):     86.715 s ±  3.165 s    [User: 276.518 s, System: 14.973 s]
  Range (min … max):   82.019 s … 91.968 s    10 runs

git checkout ramsay_clean_up_rspotify 
> hyperfine --warmup 2 'cargo clean && cargo build --no-default-features --features client-reqwest' -i
Benchmark #1: cargo clean && cargo build --no-default-features --features client-reqwest
  Time (mean ± σ):     88.646 s ±  1.688 s    [User: 285.274 s, System: 15.676 s]
  Range (min … max):   84.932 s … 90.441 s    10 runs

And I think this PR is ready to be reviewed again :)

src/model/enums/misc.rs Outdated Show resolved Hide resolved
src/model/enums/misc.rs Outdated Show resolved Hide resolved
src/model/enums/country.rs Show resolved Hide resolved
src/model/enums/types.rs Show resolved Hide resolved
src/model/enums/mod.rs Outdated Show resolved Hide resolved
src/model/enums/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@marioortizmanero marioortizmanero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to be merged, if that's what you want.

@ramsayleung
Copy link
Owner Author

Merged :)

I think I will take times to cross off more items in #127 :)

@ramsayleung ramsayleung merged commit 423982d into master Oct 27, 2020
@ramsayleung ramsayleung deleted the ramsay_clean_up_rspotify branch October 27, 2020 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants