-
Notifications
You must be signed in to change notification settings - Fork 155
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
CanonicalizeEncodingNames: fix locale-sensitive toUpperCase
#265
Conversation
val oldLocale = Locale.getDefault | ||
Locale.setDefault(new Locale("tr")) | ||
try { | ||
val (newEncoding, problem) = CanonicalizeEncodingNames.canonicalizeName("iSo-8859-1") |
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.
Unfortunately, this only tests the case conversion directly in CanonicalizeEncodingNames.canonicalizeName
:
Line 52 in 4ddd3c7
aliasToCanonical.get(Platform.toUpperLocaleInsensitive(original)) match { |
Meaning that these occurrences are untested:
Lines 67 to 71 in 4ddd3c7
private val aliasToCanonical: Map[String, String] = | |
EncodingList.canonicalToAlias.flatMap { case (canonical, aliases) => | |
aliases.map(alias => (Platform.toUpperLocaleInsensitive(alias), canonical)) | |
} ++ | |
EncodingList.canonicalToAlias.keys.map(x => Platform.toUpperLocaleInsensitive(x) -> x) |
I presume this aliasToCanonical
initialization runs at the time the object CanonicalizeEncodingNames
is loaded, so it's probably not as easy to change the default locale & re-run it dynamically as I did here with canonicalizeName
. Perhaps there's a way to do that using ClassLoader
or something similar, but I have no idea (maybe it's not worth the complexity).
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.
Yeah, I'm leaning towards it doesn't worth the complexity for the test. It looks like it works if tested manually, so seems to be good to go.
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.
Maybe worth adding this remark as a comment to the test implementation, though.
Avoid the problem with the Turkish locale that we have encountered in the past, see kaitai-io/kaitai_struct#708.
4ddd3c7
to
c00f7cd
Compare
c00f7cd
to
89226c1
Compare
Avoid the problem with the Turkish locale that we have encountered in the past, see kaitai-io/kaitai_struct#708.