search

ThreeTen / threeten

This project was the home of code used to develop a modern date and time library for JDK8. Development has moved to OpenJDK and a separate backport project, threetenbp.
http://threeten.github.io/
191 stars 37 forks source link

Better query for supported units #315

Closed jodastephen closed 11 years ago

jodastephen commented 11 years ago

Issue #312 mvoes part way to enhancing TemporalUnit.isSupportedBy, but that method is still poor and not up to standard.

If we add TemporalQuery.supportedUnits() as a query returning ChronoUnit[] then we can change to this in ChronoUnit:

boolean isSupportedBy(Temporal temporal) {
  ChronoUnit[] supportedUnits = temporal.query(supportedUnits());
  if (supportedUnits != null) {
    return supportedUnits.contains(this);
  }
  return false;

There would be no default method and other units would have to derive from ChronoUnits.

RogerRiggs commented 11 years ago

It would be more direct to just add Temporal.isSupported(Unit) similar to TemporalAccessor.isSupported(field). The suggested query is a poor choice as it would require allocating objects and using an expensive contains operation for a function that the Temporal already has direct knowledge of. It also would require explicitly listing of units that may derived from other units.

jodastephen commented 11 years ago

Adding isSupported(unit) instead seems OK to me.

jodastephen commented 11 years ago

Patch ready for review https://gist.github.com/jodastephen/5901479

RogerRiggs commented 11 years ago

With respect to isSupported(null) is it more useful to return false than to throw NPE? Otherwise it looks ok to me.

jodastephen commented 11 years ago

There is a general principle in 310 that we throw NPE in all methods taking an argument except those "is" methods returning a boolean. Same as Object.equals(null).

jodastephen commented 11 years ago

Fixed by http://hg.openjdk.java.net/threeten/threeten/jdk/rev/309941ea2062