search

tekartik / sembast.dart

Simple io database
BSD 2-Clause "Simplified" License
763 stars 63 forks source link

abstract wrapper right before christmas #371

Closed flutter-painter closed 7 months ago

flutter-painter commented 7 months ago

Hi @alextekartik,

Could you consider exposing this wrapper inside Sembast package ? This would make it easier to use dbs accross projects. The use case here is adding persistence to aptabase to store events and send them once back online. The idea is that Aptabase only exposes the eventsservices, not the database, which is passed by the client. This avoids duplicating the db tasks and lets devs set-up this service within their own state-management. Having a common wrapper class would allow devs to use a dedicated db.

alextekartik commented 7 months ago

It seems that this is really for your specific needs and how you abstract things on your own. A more generic implementation would be:

abstract class Wrapper<T> {
  final T value;
  const Wrapper(this.value);
}

I guess there would be similar issue if you use another database (hive, sqflite, isar) so I'm not sure sembast should declare a generic wrapper (Wrapper could be for anything) for its database instance. What prevent you from having this definition in your own library (that you could share between project). Unless others find it useful, I will not merge your PR...sorry!

flutter-painter commented 7 months ago

You are right, I actually implemented it yesterday within the lib. I thought it could give an example for other devs Who use sembast. Thank for looking at it though :)

flutter-painter commented 7 months ago

I rolled this out and tried to play with your generic but it does not work. I prefer mine which is Sembast DB oriented. I would find it ugly to have to publish my own package to have an abstract class of four lines. What is the problem with adding this file ?

Others might already be scratching their heads wondering what is the right way to handle multiple dbs in sembast. Unless you have a recent poll showing that they don't why not play it safe and illutrate how to do this within your repo. Already two people would find this useful. Shall I gather more and ping them here ?

alextekartik commented 7 months ago

What is the problem with adding this file ?

Sorry if it sounds hard for you but basically you are asking me to add and maintain a new API specific to your need. I did not ask to publish your own package but if you are using this in multiple projects I guess you have some git/path dependency where you can share it.

I guess that if you ask to publish the same "Wrapper" around Hive, Object, Sqlite, Isar or any package where you want to have a composition wrapper you'll get a similar response.

I don't know what is the right way to handle multiple dbs in sembast but sorry if I don't see where this 4 lines of code really help.

I did propose PRs in other repos that were sometimes rejected so I understand that it is frustrating when you spend some time and think that you have the right idea.

Development is opiniated so sorry if you spend some time on this. I also have my own wrapper around several API sometimes they look like yours, sometimes not.

Already two people would find this useful. Shall I gather more and ping them here ?

Of course if others think that it is a must have in sembast and that I'm missing something, they should participate here.