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

Force XML and CSV sources to be interpreted using the en_US locale #356

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

jenshp
Copy link
Member

@jenshp jenshp commented Apr 18, 2019

Force XML, CSV and REST sources to be interpreted using the en_US locale instead of the current value of the default_locale property.

@jonesde
Copy link
Member

jonesde commented Apr 22, 2019

Thanks Jens, overall these look like reasonable changes. Some could be simplified and avoid replacing values passed to the methods (like the Codacy review points out, I haven't started working on those cleanups but have GitHub-Codacy setup with the intent of doing this at some point). This would also reduce the patch size, ie using something like 'Locale curLocale = locale != null ? locale : getLocale();' instead of the 'if (locale == null) locale = getLocale();'.

On the overall approach I like your idea better about supporting a locale in the data file, like the standard attribute on the root element of an XML file and maybe another optional entry in the first line of a CSV file (along with entity/service name, etc). It would be nice to avoid another hard-coded value like this (in spite of all the configuration options all over the place there are still too many of these). It could default to 'en_US' if no locale specified (like other defaults elsewhere) but would be nice to support an override.

BTW, a temporary workaround that comes to mind is to set the default locale to en_US or whatever will work for the files to be loaded, then change the default locale back. For manual imports through the Tools app the current code would support it if the active user's locale is compatible with the file.

@jenshp
Copy link
Member Author

jenshp commented Apr 23, 2019

Yes, I noticed the checks with Codacy, so will work on that and update this pull request.

@jenshp
Copy link
Member Author

jenshp commented Apr 25, 2019

Just finished adding configurable options for XML, Json and CSV imports:
XML: specify locale string as entity-facade-xml.@Locale or seed-data.@Locale attributes
Json: specify global locale in first record together with dataType, or per-record (overrides jsonObj-wide global specification) as "_locale" value.
CSV: specify as third parameter on first line, after data-type

@jenshp
Copy link
Member Author

jenshp commented Sep 3, 2019

This is ready to be merged from what I can see, have been actively using it some while now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants