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

Class name IsoFields is confusing #338

Closed MenoData closed 10 years ago

MenoData commented 10 years ago

I would suggest you to think again about the name of class IsoFields in the temporal package. Reason is for example that users will not expect to find even units instead of fields there, for example QUARTER_YEARS (which is also not part of ISO8601 - strictly speaking). In general this class rather appears to be like a basket for miscellaneous temporal extensions. Maybe the class name TemporalExtensions or similar would be better, also for future enhancements.

jodastephen commented 10 years ago

IsoFields includes the ISO definition of week-based fields, hence its name.

The quarters fields only work with the ISO calendar system, hence their presence there.

Merging JulianFields and IsoFields to create a TemporalFields class is something I could possibly consider.

RogerRiggs commented 10 years ago

The contents of ISOFields were segregated from ChronoField/Unit because they did not apply to all calendars. (Not that every calendar supports every field). A classname of TemporalFields implies that they might be generally applicable. So either the class name proposed is to general or those fields and units should just be merged into ChronoField/Unit. and remove the separation (but document the limits on applicability.)

MenoData commented 10 years ago

The merging idea resulting in a unified class TemporalFields seems to be consequent and interesting. It is also somewhat similar to the collection of predefined static adjusters in the interface TemporalAdjuster.

@RogerRiggs I am not so sure that even ChronoField/Unit is applicable to all calendars. Just imagine if somebody wants to implement a Mayan calendar where the concept of weeks, months and years has no real meaning. Runtime exceptions with ChronoField/Unit are even unavoidable in this case. The whole api is strongly based on runtime exceptions (what disturbs me but cannot be changed in this stage), so users will have been accustomed to such an "unsafe" behaviour and learn which compiled code is possible at runtime and which not. Therefore I don't bother too much if the name "TemporalFields" might suggest to some users its overall applicability or not. It is also possible and necessary to document when the concrete temporal fields are applicable to any temporal object anyway.

MenoData commented 10 years ago

But one (the same) issue is also given with the name "TemporalFields" because it will contain units and not only fields (see QUARTER_YEARS). So the name "TemporalComponents" might be a better choice.

jodastephen commented 10 years ago

We discussed this today. We concluded that there was insufficient benefit to the change at this point (shortly before API freeze). The "where" question will be solved by google and stack overflow. The "ISO" question is correct because quarters only work with the ISO calendar system. We haven't got plans to add any more fields, with only Half-years being a vague possibility, and that would fit with IsoFields.

Thanks for raising the issue, as it was worth considering, but it will be staying as is.