Skip to content

Feature/provider options#23

Merged
PhenX merged 8 commits intomasterfrom
feature/provider-options
May 24, 2025
Merged

Feature/provider options#23
PhenX merged 8 commits intomasterfrom
feature/provider-options

Conversation

@PhenX
Copy link
Copy Markdown
Owner

@PhenX PhenX commented May 22, 2025

Fixes #8

Comment thread src/PhenX.EntityFrameworkCore.BulkInsert/BulkInsertProviderBase.cs
@SebastianStehle
Copy link
Copy Markdown
Collaborator

I am not the biggest fan. My code is in general provide-agnostics. When you see an Insert(...) call you don't know which provider is actually behing that. I would only have the provider specific stuff in the Context (when I create multiple contexts, one per provider).

If I have an Insert call, I would have three options:

Option 1: No provider specific option at all

Moves the options to the UseXYZCall()

Option 2: Have the opportunity to provide all options

e.g.

await context.Insert(..., options => {
   options.MySql.Xyz = 
});

But this would create some coupling in my repositories and domain layer.

Option 3: Have dedicated overrides

e.g. InsertMySqlAsync(..., MysqlOptions)

If a provider wants his own option, it should also provide it own overrides for extension method, otherwise the whole option thing is not type safe.

@PhenX
Copy link
Copy Markdown
Owner Author

PhenX commented May 22, 2025

Even if we can do this ?

image

Of course, we also can provide option 3

fabien.menager added 2 commits May 22, 2025 17:56
# Conflicts:
#	src/PhenX.EntityFrameworkCore.BulkInsert.MySql/MySqlBulkInsertProvider.cs
#	src/PhenX.EntityFrameworkCore.BulkInsert.SqlServer/SqlServerBulkInsertProvider.cs
#	src/PhenX.EntityFrameworkCore.BulkInsert.Sqlite/SqliteBulkInsertProvider.cs
#	src/PhenX.EntityFrameworkCore.BulkInsert/Abstractions/IBulkInsertProvider.cs
#	src/PhenX.EntityFrameworkCore.BulkInsert/BulkInsertProviderBase.cs
#	src/PhenX.EntityFrameworkCore.BulkInsert/PhenX.EntityFrameworkCore.BulkInsert.csproj
@SebastianStehle
Copy link
Copy Markdown
Collaborator

I think this would also be fine. I would go with Option 1 and your alternative then.

Comment thread src/PhenX.EntityFrameworkCore.BulkInsert.MySql/MySqlBulkInsertProvider.cs Outdated
fabien.menager added 4 commits May 23, 2025 17:29
# Conflicts:
#	src/PhenX.EntityFrameworkCore.BulkInsert.MySql/MySqlBulkInsertProvider.cs
#	src/PhenX.EntityFrameworkCore.BulkInsert.SqlServer/SqlServerBulkInsertProvider.cs
#	src/PhenX.EntityFrameworkCore.BulkInsert.Sqlite/SqliteBulkInsertProvider.cs
#	src/PhenX.EntityFrameworkCore.BulkInsert/BulkInsertProviderBase.cs
#	src/PhenX.EntityFrameworkCore.BulkInsert/Extensions/DbSetExtensions.cs
#	tests/PhenX.EntityFrameworkCore.BulkInsert.Tests/Tests/Basic/BasicTestsBase.cs
@PhenX PhenX marked this pull request as ready for review May 24, 2025 06:38
Copy link
Copy Markdown
Collaborator

@SebastianStehle SebastianStehle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one comment that has not necessarily something todo with this PR.

@PhenX
Copy link
Copy Markdown
Owner Author

PhenX commented May 24, 2025

Ok, I'll move it in the future, to the root namespace

@PhenX
Copy link
Copy Markdown
Owner Author

PhenX commented May 24, 2025

Is it ok for you ? it seems I cannot merge it without your approval 😅

@SebastianStehle
Copy link
Copy Markdown
Collaborator

SebastianStehle commented May 24, 2025

Yes. But please make squash merges. my commits are not very clean and now the history is a little bit broken. Have not realized it before.

I have no button to press, no idea why you cannot merge. I have resolved all conversations. Perhaps tis was the culprit?

Copy link
Copy Markdown
Collaborator

@SebastianStehle SebastianStehle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@PhenX PhenX merged commit 8a6b02f into master May 24, 2025
1 check passed
@PhenX PhenX deleted the feature/provider-options branch May 24, 2025 08:47
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.

Expose more SqlBulkCopy/BinaryImport options

3 participants