-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
Add sqlite resource #165
base: master
Are you sure you want to change the base?
Add sqlite resource #165
Conversation
Ughuuu
commented
Feb 23, 2024
•
edited
Loading
edited
- Fixes Database as resource #164
893443c
to
34c297c
Compare
@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 Is this something that can be supported in some way? |
I wouldn't know. From my knowledge I know it's possible to use hardcoded extensions. |
Oh, I see, right now default extension is a property on the node. Hm. |
But for now, as to not make breaking changes, I can just add a defaultExtension property on the project settings inside: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Ughuuu 😄
I've had some time to review the MR and here are my comments:
- Move the new source files to a new
resource/
-folder (similar to thevfs/
- andsqlite/
-folders) - For some reason the new
default_extension
-setting only shows up if you enable the "Advanced Settings" button. Is it possible to make it so that it shows up without having to enable this? - Is there any reason why the new default_extension property in the ProjectSettings should be a PackedStringArray? I think I would prefer to have it as a simple String instead (for now).
- I don't think there's any point in having the
get_content()
-method in the SQLiteResource? Or is there some hidden mechanism that I don't know about? Same with the constructor and destructor in the SQLiteResource class?
I can see that integrating this resource even tighter into the main SQLite would definitely make sense, but that would break backwards compatibility.
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 😄 |
ea8ee3e
to
c13ad13
Compare