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

Consider making the cache file a feature #135

Closed
marioortizmanero opened this issue Oct 10, 2020 · 6 comments
Closed

Consider making the cache file a feature #135

marioortizmanero opened this issue Oct 10, 2020 · 6 comments
Labels
discussion Some discussion is required before a decision is made or something is implemented help wanted Extra attention is needed
Milestone

Comments

@marioortizmanero
Copy link
Collaborator

The cache file feature is something I'm not sure how I feel about. While it's definitely useful, I'm not sure if it should be the default behaviour. It could be moved into a feature, which when activated, would use the cache file anywhere possible. Or instead of having do_something and do_something_without_cache, it could be do_something and do_something_with_cache.

What do you think? I need some opinions on this. I don't actually plan on changing anything but I want to propose any breaking changes before the next release.

@marioortizmanero marioortizmanero added help wanted Extra attention is needed discussion Some discussion is required before a decision is made or something is implemented labels Oct 10, 2020
@ramsayleung
Copy link
Owner

ramsayleung commented Oct 11, 2020

Yes, I think it's more proper to have a switch to control how does it work. My original thought was using cache_path as a trigger, as what I commented in #129:

    #[builder(default = "PathBuf::from(\".spotify_token_cache.json\")")]
    pub cache_path: PathBuf,
	// I am thinking about whether it's better to make cache_path optional
    // so we can decide to whether save the token into file or not by checking 
    // if cache_path exists, so we can remove function get_token_without_cache

Now, probably it's more suitable to make it as a feature, switching on compile time instead of runtime, as what you suggests.

@ramsayleung
Copy link
Owner

Any other proposals? I am gonna to start to work on this feature on this weekend as what @marioortizmanero suggests, makes cache file as a new feature.

@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Oct 24, 2020

Can you cross off more things from the meta issue before this? I would wait a bit more before this, let's see if anyone opposes the idea.

@ramsayleung
Copy link
Owner

Sure! We could leave this issue alone for a while.

@marioortizmanero marioortizmanero added this to the v0.11 milestone Oct 27, 2020
@marioortizmanero marioortizmanero changed the title Consider making the cache file a feature or not default Consider making the cache file a feature Dec 28, 2020
@marioortizmanero
Copy link
Collaborator Author

This has been inactive for a while and noone has left their opinion so I would say we can introduce the cache-file feature. Should it be activated by default?

Also, do you still want to work on it @ramsayleung?

@ramsayleung
Copy link
Owner

Should it be activated by default?

I think it would be good to active this feature by default, just makes rspotify work as before.

Also, do you still want to work on it @ramsayleung?

Not yet, I am working on this issue: #163, I would like to work on this feature after merging the PR of #163, and we could say we have crossed off most of TODO items in #127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Some discussion is required before a decision is made or something is implemented help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants