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

Separate crates #199

Merged
merged 74 commits into from
Jul 8, 2021
Merged

Separate crates #199

merged 74 commits into from
Jul 8, 2021

Conversation

marioortizmanero
Copy link
Collaborator

@marioortizmanero marioortizmanero commented Apr 13, 2021

Description

Some work towards #191. I've ended up separating the crates but leaving the auth, http and client ones mixed under rspotify_client until #173 is worked on.

Motivation and Context

See #191

Dependencies

Should be the same

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

This change means that types like FullAlbum in the model now have to be public.

How Has This Been Tested?

The previous tests still work now. Same goes for the examples (which by the way I had to fix because they weren't working)


Closes #191

@marioortizmanero marioortizmanero marked this pull request as draft April 13, 2021 16:09
@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Apr 13, 2021

I think the best organization for our crates will be the following:

  • src/: the Spotify client, with the endpoints and everything needed to access the API
  • rspotify-model/: with the model
  • rspotify-http/: with the HTTP client abstraction features
  • rspotify-auth/: with the Auth functionalities, exclusively

In this PR rspotify-http, rspotify-auth and src are inside src because they are too tied together. I was going to make the client a separate crate, like rspotify-client but figured it's not really worth it. I can understand that someone might want to use rspotify-model or rspotify-http on their own, but using rspotify-client would be just like using rspotify, which is just confusing, in my opinion.

So right now, it's pretty much what #191 was proposing.

@marioortizmanero marioortizmanero marked this pull request as ready for review April 13, 2021 17:29
@marioortizmanero marioortizmanero changed the base branch from master to ramsay_fix_lint_error April 13, 2021 17:35
@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Apr 13, 2021

This is now ready for review. Please @ramsayleung take a look at #166 after merging this; it's been open for a while and I would like to finish it before #173 if possible.

I'm trying to clean up the CI while I'm at it and... can you remind me why are we cross-compiling at all? It's not like Rspotify implements anything different for any architecture, so if that failed it would actually be Rust's fault. We'd get rid of six currently unnecessary checks in the CI; I could remove build entirely because test already builds Rspotify.

Also, I've tried to use cargo test --workspace --no-deafult-features and I can't get that to work in any way, so I've made a separate check for the rest of the crates. cargo clippy --workspace --no-default-features does work, though; it's probably got to do with the dependencies, as rspotify-macros depends on rspotify with the default features.

Edit: My theory is correct, if I remove rspotify from rspotify-macros' dev dependencies, it does work. Not sure if I should do so or just use the workaround with two checks for testing.

Base automatically changed from ramsay_fix_lint_error to master April 14, 2021 15:01
@ramsayleung
Copy link
Owner

Since rspotify-http, rspotify-auth are too tied together, and we have to keep working on the separating crates after #173 is done, would you like to consider working on this PR after #173 and #166 merged, so we don't have to change it for twice, just to reduce some unnecessary context-switch work

Please @ramsayleung take a look at #166 after merging this; it's been open for a while and I would like to finish it before #173 if possible.

Sorry for this, I don't realize that it's ready to review, I will check it on the weekend :)

@marioortizmanero marioortizmanero marked this pull request as ready for review July 7, 2021 15:18
@ramsayleung
Copy link
Owner

I am not sure if this PR ready to merge, even though I have merged all separated PRs

@marioortizmanero
Copy link
Collaborator Author

Yes, you can merge it.

@ramsayleung ramsayleung merged commit e035756 into master Jul 8, 2021
@ramsayleung ramsayleung deleted the separate-crates branch July 8, 2021 09:48
@ramsayleung
Copy link
Owner

Finally, it takes so long, but we reached it :)

@marioortizmanero
Copy link
Collaborator Author

Yes I can't believe it! We're getting closer to v0.11 :)

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