-
Notifications
You must be signed in to change notification settings - Fork 349
Customizable CultureInfo at field and record level. #306
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
Conversation
… of the FieldConvert attribute, instead of an hardcoded decimal separator
…options at the record and field levels
|
Salut Jean-Christophe Thank you for your well documented and explained pull request. Thank you for your patience, Matthias |
mcavigelli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Salut Jean-Christophe
Thank you for your useful pull request.
The unit tests were really useful to understand the feature you have implemented.
There are only two small changes I (on behalf of ReSharper:-)) would like you to do, see the inline
comments.
However for your next pull request may I ask you to split your commits into
- file formatting commits
- refactoring commits (renaming of variables and methods, extracting of methods)
- changes of functionality
Smaller change sets per commit will help reviewers to understand the code quicker.
Thank you again for your contribution to FileHelpers and thank you for your patience,
Matthias
| /// Default culture name used for each properties if no converter is specified otherwise. If null, the default decimal separator (".") will be used. | ||
| /// </summary> | ||
| public string DefaultCultureName { get; private set; } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto propety can be get only.
| protected TypedRecordAttribute() {} | ||
| protected TypedRecordAttribute(string defaultCultureName) | ||
| { | ||
| this.DefaultCultureName = defaultCultureName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"this" qualifier is not necessary
Currently, most Field converters expose a way to set the decimal separator. The user can choose either the dot "." or the comma ",", and the group separator is set to the other one. Therefore, it is not possible to use the French culture, as in this culture, the "dot" is not allowed as a group separator. Even if the developer sets the separator to the comma, it does not prevent from using an english compliant file.
In the ConverterHelpers.cs file, more precisely in the createCulture method, if the decimal separator is set to a comma, the group separator is hardcoded as the dot, which result in an invalid culture behaviour.
We wanted to find a way to specify that our files use French culture without breaking existing code, so made two changes to the code :
DelimitedRecordAttributesandFixedLengthRecordAttributeto specify a culture that will be used on each converter. If it is not specified, the behaviour remains exactly the same as before (it uses the current culture with a dot as a separator). It should not change anything for existing code. The user can even specify another culture on specific fields if he wants to mix cultures.We added unit tests and usage demo in the FileHelpers.Tests.Converters.DefaultCultureInfo class.