fix: CSV extenstion uses invariant culture #156
Conversation
Added support for specifying culture info for csv reader and writer.
| { | ||
| case "invariantculture": return CultureInfo.InvariantCulture; | ||
| case "current": return CultureInfo.CurrentCulture; | ||
| default: return CultureInfo.GetCultureInfo(this.Culture); |
There was a problem hiding this comment.
Great addition, I know this is an edge case, but it's entirely possible someone specifies an invalid Culture which would cause this line to fail. Can you implement an IValidatableObject that validates the this.Culture property is a valid culture and fail validation if not. There are a couple examples in other extensions you can follow. CultureInfo doesn't have a TryParse, it looks like probably checking against the systems Cultures would be a possible approach: https://stackoverflow.com/a/12345652.
There was a problem hiding this comment.
I chose to use a simple Try/Catch on the Validate method to catch invalid cultures. The GetCultureInfo() will still throw an exception when used directly.
Extensions/Csv/README.md
Outdated
| @@ -35,9 +36,17 @@ Source supports an optional `Delimiter` parameter (`,` by default) and an option | |||
|
|
|||
| Sink supports an optional `Delimiter` parameter (`,` by default) and an optional `IncludeHeader` parameter (`true` by default) to add a leading row of column names. | |||
|
|
|||
| Formatting options, or locale, can be set with an optional `Culture` setting (`"Invariant"` by default). | |||
There was a problem hiding this comment.
It might be confusing to not have the settings match the CultureInfo properties. InvariantCultureand CurrentCulture. The way the switch is currently implemented "invariantculture" works but "invariant" won't. For consistency, I'd choose either "Invariant"/"Current" or "InvariantCulture"/"CurrentCulture".
There was a problem hiding this comment.
The switch case accepts both, but I have specified only InvariantCulture/CurrentCulture in the README file.
- Added `IValidatableObject` to `CsvWriterSettings`. - Updated Culture to accept both `Invariant`/`Current` and `InvariantCulture`/`CurrentCulture`. - Specified only the latter in README. - Added unit testing for `CsvWriterSettings`.
There was a problem hiding this comment.
Looks great, thank you for adding unit tests as well. @bowencode can you take a look at this PR?
Fixes #155
Added support for specifying culture info for csv writer.