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

Make cache file a optional feature #178

Closed
wants to merge 11 commits into from

Conversation

ramsayleung
Copy link
Owner

@ramsayleung ramsayleung commented Jan 16, 2021

Description

Trying to fix #135 #96, make cache file as a optional feature.

Motivation and Context

The cache file feature is useful, but the API surface is some kind of verbose, like do_something and do_something_without_cache. The developers have to explicitly choose cache function or without_cache function to control the way of how does the cache file work? It would be more ergonomic to make cache file as a feature to offer the right to developers to control how does it work ?

Dependencies

Type of change

Please delete options that are not relevant.

  • 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

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

Test cache-file feature by hand:

  • request_client_token:
# Check if cache file is generated, expect to be nothing
cargo run --example track --no-default-feature --features="env-file cli client-reqwest request-default-tls" 

# Check if cache file is generated, expect that `.spotify_token_cache.json` is saved
cargo run --example track --no-default-feature --features="env-file cache-file cli client-reqwest request-default-tls"
  • request_user_token:
# Check if cache file is generated, expect to be nothing
cargo run --example me --features="env-file cli client-ureq ureq-rustls-tls" --no-default-features

# Check if cache file is generated, expect that `.spotify_token_cache.json` is saved
cargo run --example me --features="env-file cli client-ureq ureq-rustls-tls cache-file" --no-default-features

@ramsayleung ramsayleung changed the title [WIP]Make cache file a optional feature Make cache file a optional feature Jan 21, 2021
@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Jan 21, 2021

I was thinking that if we end up accepting the architecture at #173 we could just make a trait for cached requests and another for regular ones. That would avoid all these conditional expressions, which look quite ugly to me and make the code harder to follow and read. But I'm still not sure if fully trait-based code is that common in Rust, I have to investigate more. Unfortunately I won't be able to make much progress on this until February because I'm on finals right now.

What do you think?

@ramsayleung
Copy link
Owner Author

That would avoid all these conditional expressions, which look quite ugly to me and make the code harder to follow and read.

Yes, it looks ugly to me either, but it's the most straight way coming to my mind.

But I'm still not sure if fully trait-based code is that common in Rust, I have to investigate more.

Great to see that, IMHO, fully trait-based code looks like interface implementation in OOP, I am also not sure that it's common in Rust

Unfortunately I won't be able to make much progress on this until February because I'm on finals right now.

It's understandable, I will wait for your new progress :)

@ramsayleung ramsayleung marked this pull request as draft January 22, 2021 01:39
@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Feb 16, 2021

I think we could try to implement without features, as I wouldn't want to have too many of them. Although they make the library more modular and less bloated, configuring them can be complicated and limits it to a compile-time configuration. The code can also get quite ugly this way (take this PR as an example, which is filled with #[cfg] statements.

Not sure how easy it is to implement this but it might be a nice idea imo. This would be specially interesting because that way we could have three types of tokens:

  • Regular token
  • Cached token
  • Refreshing token

Which could be combined as well -- e.g. a cached refreshing token. But that could require reading about more sophisticated feature-management systems, like https://www.reddit.com/r/rust/comments/lkdw6o/designing_a_new_architecture_for_rspotify_based/gnjypic/, so it might take a while.

@marioortizmanero marioortizmanero deleted the ramsay_cache_file_as_a_feature branch July 8, 2021 11:43
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.

Consider making the cache file a feature
2 participants