Skip to content
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

Rename NpgsqlUuid7ValueGenerator to NpgsqlSequentialGuidValueGenerator and make it public #3255

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

roji
Copy link
Member

@roji roji commented Sep 1, 2024

EF only automatically configured client-side GUID generation for key GUID properties; other non-key GUID properties do not get configured by default. Since users may want to explicitly configure that behavior (see explicit call-out on this in the docs), the new UUID7 generator should be public and more properly named.

@ChrisJollyAU @Timovzl does this (and the new naming) make sense to you?

Follow-up to #3249

@ChrisJollyAU
Copy link
Contributor

And I just moved it from public to internal from your suggestion

EF's GuidValueGenerator is fully public since it's meant to be referenceable by external providers, i.e. it's infrastructure; whereas types inside EFCore.PG aren't... So I think it's indeed better to make the generator pubternal (by moving it into the Internal namespace) - we don't want people to start using it for whatever purpose, it's just an internal implementation detail of the provider.

Anyway if you want to allow for that scenario should be fine

@roji
Copy link
Member Author

roji commented Sep 1, 2024

And I just moved it from public to internal from your suggestion

Yeah, I indeed didn't have the non-key scenario in mind...!

@roji roji merged commit 36295e9 into npgsql:main Sep 1, 2024
15 checks passed
@roji roji deleted the RenameGuidGenerator branch September 1, 2024 16:58
@Timovzl
Copy link

Timovzl commented Sep 3, 2024

@ChrisJollyAU, @roji, should we now provide appropriate summaries on the public members, instead of the pubternal summaries?

@roji I do like the naming consistency with EF's counterpart, but I dislike the "sequential" claim so long as the implementation generates nonsequential IDs intra-millisecond. The name adds to the astonishment when it comes to that property. For your consideration.

@roji
Copy link
Member Author

roji commented Sep 3, 2024

Sure, feel free to submit a PR if you want to add docs (though I think there's little value in it).

I really think it's fine to use the word sequential here; that's not the same as saying "strictly monotonic" - the values produced really are sequential (in the sense that they're not random).

@Timovzl
Copy link

Timovzl commented Sep 3, 2024

Technically, I disagree with the statement that the values really are sequential: if you produce a handful at once, their order literally will be random, not sequential. But I mainly wanted to check that you're aware and satisfied.

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.

3 participants