unity-sqlite-net icon indicating copy to clipboard operation
unity-sqlite-net copied to clipboard

Custom Table Name, Priority TableAttribute.Name > CustomName > MappedType.Name

Open ChaosVan opened this issue 10 months ago • 6 comments

Hello, GilZoide~

I have implemented the custom TableName functionality locally and am now sharing it with you. Please review to see if a merge is needed. Additionally, I made some adjustments to the formatting in SQLite.cs; feel free to reformat it according to your preferences.

Best regards!

ChaosVan avatar Feb 19 '25 04:02 ChaosVan

Hey @ChaosVan, thanks for the PR.

What's your use case for this again? Isn't [Table(...)] enough? By the way, the fact that you reformatted the whole SQLite.cs source file makes it super difficult to review your changes.

gilzoide avatar Feb 19 '25 12:02 gilzoide

I have reformatted the code by Tab (instead of 4 space)

On my case, there are some requirements: some tables have the same structure but different names, and when calling CreateTable function, I need to pass the same Type but different Names.

ChaosVan avatar Feb 20 '25 02:02 ChaosVan

I have reformatted the code by Tab (instead of 4 space)

There are still lots of code formatting changes, but it's ok, I'm able to see the relevant changes directly from the relevant commits.

some tables have the same structure but different names, and when calling CreateTable function, I need to pass the same Type but different Names.

Got it, that's a legit use case. But thinking of each table as a separate model in the database, doesn't it make sense to have one class for each model, even though they contain the same fields? That's what most ORM libraries do.

If you're worried you'll have to repeat yourself over and over again for tables with similar content, maybe you could use a common base class and derive it to the desired models, each one with the correct table name, wouldn't this work? Then for the insert/update/delete/query calls you won't need to specify the base type and custom table name, just pass the child class directly and it will reference the correct table automatically. What do you think?

gilzoide avatar Feb 21 '25 10:02 gilzoide

Thank you very much. I understand the ORM approach, but in practice, I might not know the table names in advance—they could be dynamically determined at runtime when I need to create such a table.

ChaosVan avatar Feb 21 '25 10:02 ChaosVan

I might not know the table names in advance—they could be dynamically determined at runtime when I need to create such a table

Ok, now that's an unusual requirement. But it's legit, I agree it can be useful.

For the sake of API stability, I'll ask you to create new method overloads accepting the tableName argument instead of just adding it with default value to the existing overloads. Adding an argument with default value doesn't break code calling these methods directly like db.Find<T>(pk), but it does break code using the method group as delegates, like Func<object, T> = db.Find<T> or pkList.Select(db.Find<T>).

I also don't like much the fact that you reformatted all code in the file, I prefer if this PR only had the relevant changes in API / implementation. We can/should reformat the code, but elsewhere, likely in a separate PR. Even after reindenting the file with tabs, there are still lots of lines changed that are not relevant for the new functionality.

gilzoide avatar Feb 22 '25 14:02 gilzoide

I tried to revert the code to its pre-Format state, but it was quite cumbersome... If this use case isn't common, perhaps I can wait until you apply the Format to the main branch, then merge it into my branch and finally attempt to submit a PR.

ChaosVan avatar Feb 24 '25 06:02 ChaosVan