Skip to content

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

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ChaosVan
Copy link

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!

@gilzoide
Copy link
Owner

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.

@ChaosVan ChaosVan closed this Feb 20, 2025
@ChaosVan ChaosVan reopened this Feb 20, 2025
@ChaosVan
Copy link
Author

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.

@gilzoide
Copy link
Owner

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?

@ChaosVan
Copy link
Author

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.

@gilzoide
Copy link
Owner

gilzoide commented Feb 22, 2025

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.

@ChaosVan
Copy link
Author

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 ChaosVan closed this Feb 24, 2025
@ChaosVan ChaosVan reopened this Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants