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

DateTimeZone.IsFixed possibly shouldn't be public #62

Closed
GoogleCodeExporter opened this issue Mar 15, 2015 · 11 comments
Closed

DateTimeZone.IsFixed possibly shouldn't be public #62

GoogleCodeExporter opened this issue Mar 15, 2015 · 11 comments
Labels
bug

Comments

@GoogleCodeExporter
Copy link

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

DateTimeZone.IsFixed doesn't seem like it's particularly useful to most users - 
and it's not something that I imagine we want people to call in general, so its 
presence in the public API is somewhat confusing.

On the other hand, it is useful information for CachedDateTimeZone (which is 
internal).  We could make IsFixed internal (or possibly protected internal), 
which has the only disadvantage of making it difficult to write 
CachedDateTimeZone externall.

Original issue reported on code.google.com by malcolm.rowe on 18 Apr 2012 at 9:06

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

I think it's potentially useful for users. In particular, if they know it's 
fixed, they know they never need to worry about daylight saving transitions... 
which means they will always be able to unambiguously convert from local to 
zoned date/times.

It's possibly relevant (though certainly not a clincher) that all of 
java.util.TimeZone, org.joda.time.DateTimeZone, and System.TimeZoneInfo all 
support this. We could always make it internal and make it public if requested, 
of course...

Original comment by jonathan.skeet on 18 Apr 2012 at 6:48

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

If you meant JDK's TimeZone.useDaylightTime(), it looks like that ignores 
historical data, so the guarantee you mention wouldn't hold. (That may be true 
of the other two you mention - the documentation isn't clear.)

Stack Overflow has precisely one reference to Joda's isFixed() method that I 
could find, and that from someone trying to do the wrong thing. I'd be inclined 
to make this protected internal until we see a request for the functionality.

Original comment by malcolm.rowe on 18 Apr 2012 at 8:31

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

Jon and I looked at _Joda-Time_ usage in a large codebase, and found only 
(non-test) two references to isFixed:

- one was iterating all known timezones and attempting to work out for each a 
reasonable "standard" and "daylight" offset that could be said to represent the 
zone.  Here, isFixed was used as an optimisation, but was not really required.
- the other was attempting to identify a "canonical" timezone that represented 
a given offset; i.e. given simply the offset +0400, return any known timezone 
that had that fixed offset (i.e. that did not observe DST).

The latter is the only use we've seen that would actually require IsFixed.

Original comment by malco...@google.com on 19 Apr 2012 at 2:10

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

Fixed in revision e6937fe3099d.

Original comment by jonathan.skeet on 19 Apr 2012 at 2:13

  • Changed state: Fixed
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

I was getting ready to use this, so I'm glad I came across this issue. For one, 
I don't want to use a method that won't be public in a future release, but it 
also doesn't work the way I thought it did. Take Japan Standard Time (JST), for 
example. It doesn't use DST, so I would expect isFixed to return true, but as 
was previously noted, it takes historical data into account. JST observed DST 
from 1948-51, so it returns false.

Original comment by gthazm...@gmail.com on 30 Apr 2012 at 5:11

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

Thanks for the comment on this - it's very useful feedback.

The better alternative you're looking for is probably to call 
DateTimeZone.GetZoneInterval for some appropriate instant (e.g. "the start of 
the year 2000") and then check how far back and forward that interval goes - in 
particular, if it ends to infinity going forward (if (zoneInterval.End == 
Instant.MaxValue)), then you know there won't be any transitions after whatever 
instant you choose.

Original comment by jonathan.skeet on 1 May 2012 at 6:55

@malcolmr malcolmr added the bug label Mar 15, 2015
@Dawood-ibn-Kareem
Copy link

@Dawood-ibn-Kareem Dawood-ibn-Kareem commented Jun 26, 2019

Oh, this is kind of annoying. I wanted to use something like IsFixed() or an equivalent.

I'm not convinced by Jon's remark from 1 May 2012. Consider a location that has never had daylight savings, and never intends to, but DOES intend to change its offset in the near future; such as Samoa in 2011, or North Korea in 2018. I want to be able to take a time zone for such a place, and find out whether it uses DST - but something like if(zoneInterval.End == Instant.MaxValue) as per Jon's advice gets it wrong.

Please can we have IsFixed() back? I'm not sure why any library provider would EVER remove a method like this. Even if only three people in the world have a use for it, going to the trouble of making the method private, or protected internal or whatever, just to inconvenience those three people, seems like too much trouble to go to.

@jskeet
Copy link
Member

@jskeet jskeet commented Jun 26, 2019

@Dawud-xx: You've demonstrated why it's a bad idea. My advice would give the same result as IsFixed - false - for the examples you've given. A time zone which changes its offset is not fixed. If you want to find out whether it has/will ever observe DST, that's a different question - and one which is answered in a different way, by checking whether there are any zone intervals where ZoneInterval.Savings is non-zero.

Having a property which is misunderstood by users is worse than not having the property at all.

@Dawood-ibn-Kareem
Copy link

@Dawood-ibn-Kareem Dawood-ibn-Kareem commented Jun 26, 2019

Thanks, Jon, for your speedy answer.

OK, I might go with the check you suggested - checking whether there are intervals for which ZoneInterval.Savings isn't zero. It's not really what I want either - I'm thinking of the case described by another user, where Japan's historical use of DST would show up.

What I really want is "does this zone use DST under its current legislation". So isSubjectToDST might be a better way of phrasing this than IsFixed - because "fixed" could be understood in different ways as you've pointed out (your understanding differed from mine).

In other words, I want something that would return

  • true for Sydney, Australia - because although it's currently winter in Sydney, they had DST a few months ago, and they'll be having DST again in a few more months;
  • false for Tokyo, Japan - because although they had DST in the distant past, nobody really cares.

I guess I could try zoneInterval.End < Instant.MaxValue && zoneInterval.Duration.TotalDays < 365 or something, for the time zone's current zone interval, in the hope that if you've changed your offset twice within a year, it's because of DST, not because of an indecisive government. It does seem to me like this is an actual functional gap in Noda.

@jskeet
Copy link
Member

@jskeet jskeet commented Jun 26, 2019

What I really want is "does this zone use DST under its current legislation".

That's probably a matter of looking at "all zone intervals from now onwards" (rather than historically) then. Although you'd need to think about the case where a time zone doesn't currently use DST, but plans to do so in (say) 5 years time.
What about the situation where a time zone is currently in standard time and would be in standard time even if it were continuing to observe DST, but DST has been abolished from now onwards?

It does seem to me like this is an actual functional gap in Noda.

I really don't think so - it's a matter of defining exactly what you mean, and then using the existing API. That's much better than providing specific APIs for:

  • Does this time zone have a single fixed offset for all time
  • Does this time zone have any zone intervals observing DST
  • Does this time zone have any future zone intervals observing DST
  • Is this time zone currently in a position of observing DST in the "near future" (whatever that means)

Oh, and then there's the idea of "permanent summer time" which Europe is likely to adopt, where Savings may end up being permanently 1 hour. No changes to the offset over time, but it's always "in daylight savings". Different applications might want to do different things with that.

Noda Time accurately represents the data from IANA - admittedly not including the actual Zone and Rule lines, but I don't think you'd want that either.

I suspect this is mostly a matter of you needing to be precise about your requirements before coding - and that's something Noda Time actively encourages, because the precise needs of different applications are different.

@Dawood-ibn-Kareem
Copy link

@Dawood-ibn-Kareem Dawood-ibn-Kareem commented Jun 26, 2019

Thanks Jon. I actually thought my requirement was clear, until I read what you wrote about Europe's intended permanent summer time. I have no idea what my application should do with that - although I'm struggling to see whether there's any practical difference between adopting permanent summer, and having permanent non-summer with a different offset. I might have to cross that bridge if and when it happens.

I guess I have a notion in my head of a time zone's "current set of rules" - meaning the currently legislated algorithm for determining if and when DST starts and ends. I think of North Korea's recent change as being a change to the rules. Europe going to permanent summer time would also be a change to the rules. The more mundane starting and ending of DST within a particular jurisdiction, not so much. I guess this particular notion isn't really captured by IANA, so it's unreasonable of me to expect to find it in Noda.

Thank you, Jon, for taking the time to discuss this with me. I can imagine how busy you might be, and I really appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.