Hi, Using the ".getDefault()" values sounds good, as this way we run less risk of breaking existing stuff.
But overall it would be good if we can get back to a "green" build again somehow first, I tried to see what changes are necessary right now to make tests work, but the tests all are quite complicated and not easy to fix for me. Should we take a step back and revert some of those "forbidden-api-check" changes and do them in a more step-by-step manner? Dominik. On Thu, Sep 3, 2015 at 5:10 PM, Andreas Beeker <kiwiwi...@apache.org> wrote: > Hi, > > I have a lot of test errors and already fixed quite a lot, but haven't > committed them yet. > > Short version: > how about ... instead of using Locale.ROOT and Timezone UTC, > the initial value of the ThreadLocal return Locale.getDefault() and > Timezone.getDefault() > and can be set if needed ... so the handling is similar to older versions? > > Long version: > When I've started to fix the forbidden api errors, I've only touched places, > where I was quite sure that Locale.ROOT won't do any harm. > But of course now we also have to face the other issues... > > The reason for having a thread local is ...: > - that having the timezone/locale as an additional parameter would lead to a > lot of methods > to be marked deprecated (at least I would do it...) > - I've read about use-cases where workbooks from different time zones need to > be processed, so it's not only the current environment time zone > > - "A ThreadLocal can lead to hard-to-find errors ...": > I think the hardest part is, making the new handling public. > Another problem I see with thread locals is, the reuse of servlet threads. > So the usage pattern would need to restore the original setting in a > finally block. > > - "... in fact almost anybody would now see UTC-formatted dates instead > of local timezone, or?" > I guess so too, and the tests fail when my CET-timezone is ignored > (the DE-locale is only affecting very few cases), i.e. I have to set it > explicitly > > - "The java default is to either use the system default ..." > As noted above, I would like to provide a multi timezone/locale approach, > but > still keep a lot of the poi api as-is. > > I guess the discussion will continue for a bit, so I create a patch file > instead of committing > the changes and check your feedback then. > If you commit changes in the meantime, I'll update my version accordingly, so > don't be > blocked ;) > > Andi > > > On 03.09.2015 16:25, Uwe Schindler wrote: >> Hi, >> >> This may be only slightly related: to work around the forbidden-apis issue, >> you can still pass Timezone.getDefault() to the Java APIs - this is not >> forbidden. Forbidden APIs just wants to make sure you know what you are >> doing. By writing Timezone.getDefault() you explicitely say what you are >> doing. And maybe add a comment why you do this. The same applies for Locales >> or Charsets. E.g., At some places in Lucene we explicitely use the charset >> of the console, so we use Charset.getDefault() at all those places. >> >> Uwe >> >> ----- >> Uwe Schindler >> H.-H.-Meier-Allee 63, D-28213 Bremen >> http://www.thetaphi.de >> eMail: u...@thetaphi.de >> >> >>> -----Original Message----- >>> From: Dominik Stadler [mailto:dominik.stad...@gmx.at] >>> Sent: Thursday, September 03, 2015 4:05 PM >>> To: POI Developers List >>> Subject: Re: Time Zone Setting >>> >>> Hi, >>> >>> Sorry, but I fear that through this we change the default behavior of POI: >>> * A ThreadLocal can lead to hard-to-find errors when people use POI in a >>> multi-threading environment. While it is better in some cases than having a >>> global static, it now requires the developer to ensure that the timezone is >>> set >>> for each thread that is started without a way to set it globally. >>> * I think previously POI used the java default settings which look at the >>> operating system, this is now broken as we have "UTC" as default, which >>> potentially leads to many people seeing different results compared to >>> before, in fact almost anybody would now see UTC-formatted dates instead >>> of local timezone, or? >>> * The java default is to either use the system default timezone or if it >>> should >>> be different then use -Duser.timezone when invoking the JRE, see e.g. >>> http://stackoverflow.com/questions/2493749/how-to-set-a-jvm-timezone- >>> properly >>> >>> So my approach would be to: >>> * Not use the DateUtil.setTimezone() approach as it has quite some potential >>> for regressions but also not use TimeZone.setDefault() in unit tests at all >>> * Ensure that we set -Duser.timezone to a fixed value when running unit >>> tests in the various CI systems. We already do this for user.language and >>> user.country, so setting the timezone as well here seems a natural choice >>> >>> This way we do not change default behavior for users and still make the CI >>> runs a bit more reproducible time-wise... >>> >>> Dominik. >>> >>> > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@poi.apache.org > For additional commands, e-mail: dev-h...@poi.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@poi.apache.org For additional commands, e-mail: dev-h...@poi.apache.org
