2shady4u / godot-sqlite

GDExtension wrapper for SQLite (Godot 4.x+)
MIT License
850 stars 76 forks source link

Add sqlite resource #165

Open Ughuuu opened 5 months ago

Ughuuu commented 5 months ago
2shady4u commented 4 months ago

@Ughuuu I'll review this asap

One minor thing that I noticed is that only ".db" is a valid extension at the moment, while for SQLite, the actual extension doesn't actually matter. Some people seem to prefer using ".database" or even don't use any extension at all.

I solved this issue a long time ago by adding the default_extension property such that users can choose whatever extension they want.

Is this something that can be supported in some way?

Ughuuu commented 4 months ago

I wouldn't know. From my knowledge I know it's possible to use hardcoded extensions. One option would be to have project settings (eg. SQLite/default_extension) and have the user put there setting and then require a restart(a popup if the option requires a restart). And have it be an array, and by default be only [".db"]

Ughuuu commented 4 months ago

Oh, I see, right now default extension is a property on the node. Hm. I wonder why not just use the path variable, what is the purpose of the extension. It would simplify a bit the whole design if defaultProperty were a global setting instead of a node setting.

Ughuuu commented 4 months ago

But for now, as to not make breaking changes, I can just add a defaultExtension property on the project settings inside: filesystem/import/sqlite/default_extension

Ughuuu commented 4 months ago

Just so that if someone has files with multiple extensions, eg .db and .database its easier to support this now rather than later have a breaking change.

For rest i will check and implement feedback.

Agree with integration, i am thinking of making a higher level node, eg sqlite viewer, where you put a sqliteresource and you can view database schema and values with pagination, maybe.

Maybe we can make another property also on sqlite eg sqlite_resource and keep the path also. And have if you put path that takes precedence(so its also backwards compatible)

2shady4u commented 4 months ago
  • For default_extension(maybe i can rename to default_extensions:

Just so that if someone has files with multiple extensions, eg .db and .database its easier to support this now rather than later have a breaking change.

For rest i will check and implement feedback.

Agree with integration, i am thinking of making a higher level node, eg sqlite viewer, where you put a sqliteresource and you can view database schema and values with pagination, maybe.

Maybe we can make another property also on sqlite eg sqlite_resource and keep the path also. And have if you put path that takes precedence(so its also backwards compatible)

Not super-important, but is there any way to disallow the user from inputting "" as an extension?

Tighter integration would be the next step, but I think it's best to keep the scope of this PR like it is for now though 😄