Skip to content

Change NpgsqlSnakeCaseNameTranslator to use InvariantCulture by default, accept culture parameter #5172

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 22, 2023
Merged

Change NpgsqlSnakeCaseNameTranslator to use InvariantCulture by default, accept culture parameter #5172

merged 2 commits into from
Jul 22, 2023

Conversation

yclkvnc
Copy link
Contributor

@yclkvnc yclkvnc commented Jul 21, 2023

Closes #5169

Hi @roji, I've opened the issue (#5169) with my 2nd account, but opening the PR with my main account. I have a couple of questions:

  • I've added the nullable CultureInfo to the constructor as you proposed and gave it a default value null. It will still be a breaking change but at least you can use it as before. Is it ok?
  • I've added a CultureInfo parameter to the existing TestCases and set it to null for existing cases and added spesific cases to the end of the list. I can seperate them to another TestCases list in another file if you want

@yclkvnc yclkvnc requested review from roji and vonzshik as code owners July 21, 2023 20:39
@yclkvnc yclkvnc changed the title 5169 snakecasenametranslator invariantculture Change NpgsqlSnakeCaseNameTranslator to use InvariantCulture by default, accept culture parameter Jul 21, 2023
roji
roji approved these changes Jul 22, 2023
View reviewed changes
Copy link
Member

@roji roji left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks @yclkvnc!

Yeah, the constructor change is a binary-breaking change, but not a source-breaking one; that's only really important if a 3rd-party library uses the older constructor - it's hard for me to imagine such a scenario. If we get a complaint we can always split the constructor into two to maintain backwards compat.

And thanks for the testing too!

@roji roji merged commit 83aecda into npgsql:main Jul 22, 2023
@yclkvnc
Copy link
Contributor Author

yclkvnc commented Jul 22, 2023

You're welcome and thank you :)

@azygis azygis mentioned this pull request Nov 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roji roji roji approved these changes

@vonzshik vonzshik Awaiting requested review from vonzshik vonzshik is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change NpgsqlSnakeCaseNameTranslator to use InvariantCulture by default, accept culture parameter
2 participants
@yclkvnc @roji