Skip to content

Conversation

azygis
Copy link
Contributor

@azygis azygis commented Nov 23, 2023

Culture info was introduced with #5172. Unfortunately, it wasn't implemented completely correctly - the static conversion method is exposed, and to that you cannot pass null as you'd end up with ArgumentNullException from char.ToLower().

This PR fixes that and defaults to CultureInfo.InvariantCulture as well.

@azygis azygis requested review from roji and vonzshik as code owners November 23, 2023 17:28
@NinoFloris NinoFloris added this to the 8.0.1 milestone Dec 3, 2023
@roji roji modified the milestones: 8.0.1, 8.0.2 Dec 4, 2023
@NinoFloris
Copy link
Member

@azygis I must be missing something. Why can't users of ConvertToSnakeCase not just pass it CultureInfo.InvariantCulture today?

@azygis
Copy link
Contributor Author

azygis commented Jan 31, 2024

@NinoFloris they absolutely can.

For the sake of consistency (ctor allows not passing it) and convenience (unnecessary noise in the calling code), I think it's good to have it optional on the static method as well.

In OP I mention passing null as impossible, not other cultures.

@NinoFloris
Copy link
Member

Makes sense. @vonzshik pointed out to me the xmldoc already mentions null should default to InvariantCulture. Let's merge it for 8.0.2

@NinoFloris NinoFloris merged commit 8822656 into npgsql:main Jan 31, 2024
NinoFloris pushed a commit that referenced this pull request Jan 31, 2024
…nslator.ConvertToSnakeCase (#5448)

(cherry picked from commit 8822656)
@NinoFloris
Copy link
Member

Backported to 8.0.2 via 8822656

@azygis azygis deleted the feat/fix-5172-possible-argumentnullexception branch January 31, 2024 21: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.

3 participants