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

[Auth restructure 2] Some fixes for rspotify-model and rspotify-macros #214

Merged
merged 37 commits into from
Jul 8, 2021

Conversation

marioortizmanero
Copy link
Collaborator

@marioortizmanero marioortizmanero commented Jun 19, 2021

Part 2 of the #207 split-up.

This:

  • Fixes some small things in rspotify-model
  • Fixes some small things in rspotify-macros

These things are needed later to test the crate without cyclical dependencies, and mostly clean up a few things. Both of the crates I modified should compile without problems, but not the main one.

@marioortizmanero marioortizmanero changed the base branch from master to separate-crates June 19, 2021 10:14
@marioortizmanero marioortizmanero changed the base branch from separate-crates to auth-rewrite-part1 June 19, 2021 10:14
@marioortizmanero marioortizmanero changed the title [Auth restructure 2] Some fixes for rspotify-model and rspotify-macros [Auth restructure 2] Some fixes for rspotify-model and rspotify-macros Jun 19, 2021
@ramsayleung
Copy link
Owner

It seems, some CI checks failed, it will be automatically fixed after merged part 3?

@marioortizmanero
Copy link
Collaborator Author

Yes. The CI will only work after merging the third PR

@@ -13,4 +16,3 @@ edition = "2018"
[dev-dependencies]
serde_json = "1.0.57"
chrono = { version = "0.4.13", features = ["serde", "rustc-serialize"] }
Copy link
Owner

Choose a reason for hiding this comment

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

The macro crate should relay on no dependency, since it has nothing to do with external dependencies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are dev dependencies, they're only needed for tests. You're right that we don't need chrono, though. I've removed it.

@ramsayleung
Copy link
Owner

Beside this nitpick I mention, everything looks good to me.

@ramsayleung ramsayleung merged commit bf8124b into auth-rewrite-part1 Jul 8, 2021
@ramsayleung ramsayleung deleted the auth-rewrite-part2 branch July 8, 2021 03:34
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