RFR 8146368(xs): JShell: couldn't smash the error when it's Japanese locale
bitterfoxc at gmail.com
Mon Jan 4 20:45:52 UTC 2016
Thank you for your review.
New webrev is here:
2016-01-04 12:42 GMT+09:00 Robert Field <robert.field at oracle.com>:
> Thanks, Shinya, for bringing this to the forefront. Internationalization
> is an area that has not been addressed much at this point.
> I see several places in the code that use getMessage(null) and then
> attempt to parse the result.
> Behind the API: In Unit, the case you address, the diagnostic is only used
> to extract the unresolved symbols, so it is fine to pin it to the ROOT
> Locale, though the whole parsing of diagnostics approach I took is not
> ideal. There are uses in TaskFactory which are only used for debugging.
> though they do parse the result expecting English words. The one in
> OuterWrap should also not be visible by an end-user (and, arguably should
> indeed be passed null). One approach would be to define something like
> PARSED_LOCALE = Locale.ROOT in Util, and use that throughout the jdk.jshell
> package (possibly with the exception of OuterWrap).
I've changed doing so (excepting OuterWrap).
(But I've fixed indentation around the use of #getMessage in OuterWrap)
I also except one of use in TaskFactory. Because the message is not used
for parsing and I think there is no reason to pin the locale to ROOT.
state.debug(DBG_GEN, "Pos: %d (%d - %d) -- %s\n",
(And I notice that TaskFactory#shortErrorMessage is not used anywhere)
What do you think?
> Another, much more ambitious approach would to implement the TODO comment
> in Unit.UnresolvedExtractor:
> //TODO extract from tree instead -- note: internationalization
> This would be a redesign for which I have no implementation in mind, so it
> isn't likely the right choice here.
> So, I think pulling the Locale definition up to a visible and shareable
> location is the best bet.
> Then there is the usage in JShellTool.printDiagnostics, here we clearly
> want the user's Locale (the end-user sees this message). But then the
> startsWith() search will fail in other Locales (the failure is not
> catastrophic, just extra output). I think the only thing to do for now
> would be to add a comment on the line, like: //TODO: Internationalize
I've added the comment.
As you mentioned, i18n has not been addressed yet. Can I address it after
> On 12/31/15 13:13, ShinyaYoshida wrote:
>> A HAPPY NEW YEAR!!
>> Could you review this?
>> webrev: http://cr.openjdk.java.net/~shinyafox/kulla/8146368/webrev.00/ <
>> bugs: https://bugs.openjdk.java.net/browse/JDK-8146368
More information about the kulla-dev