RFR 8146368(xs): JShell: couldn't smash the error when it's Japanese locale

[ShinyaYoshida]

Hi Robert,
Thank you for your review.

New webrev is here:
http://cr.openjdk.java.net/~shinyafox/kulla/8146368/webrev.01/

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",
diag.getPosition(),
                        diag.getStartPosition(), diag.getEndPosition(),
diag.getMessage(null));
--
(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
this?

Regards,
shinyafox(Shinya Yoshida)


> Thanks!
> Robert
>
> 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/ <
>> http://cr.openjdk.java.net/%7Eshinyafox/kulla/8146368/webrev.00/>
>> bugs: https://bugs.openjdk.java.net/browse/JDK-8146368
>>
>> Regards,
>> shinyafox(ShinyaYoshida)
>>
>
>