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

Support arbitrary storage providers #5

Open
MFlisar opened this issue Dec 10, 2020 · 8 comments · May be fixed by #6
Open

Support arbitrary storage providers #5

MFlisar opened this issue Dec 10, 2020 · 8 comments · May be fixed by #6
Labels
enhancement New feature or request

Comments

@MFlisar
Copy link

MFlisar commented Dec 10, 2020

I've made a fork here https://github.com/MFlisar/ModernAndroidPreferences and implemented an arbitrary storage layer here

What I did:

  • created an interface Storage
  • created a default storage implementation SharedPreferencesStorage which works just as your current solution
  • replaced the SharedPreferences inside your library with the Storage instance (builder, dsl, ...)

Why?

In my case I need multi process support and therefore I have written a room based preference library and I want to plug in this library into yours. Additionally, preferences are not the most modern solution for preferences, datastore is the new way - with my solution you can easily provide a storage implementation that uses the DataStore instead of the SharedPreferences in the future and still let your users choose. And of course, any user can replace the storage implementation with their own

Question

Do you think such a change (it's small and does not add any complexity to your library) does make sense and should be merged into your library? I did not adjust any tests yet but if you like the idea, I'll make those adjustments and create a pull request.

@Maxr1998
Copy link
Owner

Definitely! I already planned to integrate JetPack DataStore, however, I wanted to wait while it's still in beta. Already having an abstraction for storage providers makes a lot of sense though, so I appreciate the work you've done and would be happy if you opened a PR. I've looked at the code already and it looks mostly good, I'll probably still have some nitpicks to be adressed before merging 😄
Let's discuss those in the PR though.

@Maxr1998 Maxr1998 added the enhancement New feature or request label Dec 10, 2020
@MFlisar MFlisar linked a pull request Dec 10, 2020 that will close this issue
@MFlisar
Copy link
Author

MFlisar commented Dec 10, 2020

Done, rest can be discussed in the PR

@MFlisar MFlisar closed this as completed Dec 10, 2020
@Maxr1998 Maxr1998 reopened this Dec 10, 2020
@Maxr1998
Copy link
Owner

I prefer to keep this open until the PR is merged ^^

@MFlisar
Copy link
Author

MFlisar commented Mar 11, 2021

Any plans on implementing this soon? Or one working on my PR (together if you want)?

@Maxr1998
Copy link
Owner

Definitely still planned, but I'm absolutely packed with work (bachelor thesis) right now. Your PR looks good, but there are some things I'd like to change about it. I'll revisit it not later than May, after I finished my bachelor thesis.

@MFlisar
Copy link
Author

MFlisar commented Mar 11, 2021

Ok, no problem. Good luck with your exams.

@Maxr1998
Copy link
Owner

Just as a heads-up: I'll try to get this in soon, but probably not in the next version. Want to get some fixes out first.

@MFlisar
Copy link
Author

MFlisar commented Apr 26, 2021

No need to hurry if this is just "for me".

I've written my own library in the meantime because I wanted quite some more changes (a core that works with async operation via kotlin flows based on suspend functions, preference declarations via delegates, abstract storage with a data store based default implementation, the ability to observe change events of the storage, abstract dependencies between preferences, ...) - my screen module does even use a few small parts of your library as I've written into my credits region, so thanks for this here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants