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

Remove the concept of missing vs zero values from Period. #90

Closed
GoogleCodeExporter opened this Issue Mar 15, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@GoogleCodeExporter

GoogleCodeExporter commented Mar 15, 2015

Period differentiates between "10 years" and "10 years, zero months"; the 
latter contains the Months PeriodUnit; the former does not.

However, both instances will compare as equal, and in both cases Months will 
return zero; the only differentiation is via the Units property.

Do we actually have a sensible reason to support differentiation between 
zero-valued and absent-valued properties?  If not, we could consider simplify 
Period somewhat by removing the Units property entirely, making PeriodUnits 
simply an input parameter to Between methods (and internally, as an 
implementation detail in Period).

[In particular, the fact that equal Periods can have different Units property 
values is troubling me; if they're not intended to be interchangable, why are 
they equal?]

Original issue reported on code.google.com by malcolm.rowe on 26 Jul 2012 at 1:40

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Perhaps worth noting that another way to discriminate between periods 
containing different units is via the ToString() method, which would presumably 
need to omit all properties with zero values for consistency.

Do you think parsing text might require differentiation?

Original comment by malcolm.rowe on 26 Jul 2012 at 3:07

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

This is one area where I followed Joda Time's design without questioning it 
*too* much. Various aspects of periods have been changed, but the fundamental 
"a period knows its units" aspect remains.

It's definitely a pain in some ways. It definitely makes sense to be able to 
specify the units when computing the period to start with, but I wonder how 
often you'd need to use a period without knowing its units beforehand.

More discussion required.

Original comment by jonathan.skeet on 26 Jul 2012 at 6:12

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Per discussion:

- I would have been okay with just inferring the Units property from the 
non-zero properties, but Jon thinks that there's no real value in doing so, so 
we're going to remove the Units property entirely.
- We're also going to significantly simplify the internal representation of a 
Period, so rather than have all of a units mask, single value, and nullable 
multiple values array, we'll just use an array in all cases.  The rationale 
here is that we think it's significantly more likely that a Period will be 
constructed with multiple units than a single unit, because the single unit 
cases are mostly covered by e.g. LocalDate.PlusHours(), which doesn't use a 
Period. (This will make HasDate/TimeComponent a little slower in some uncommon 
cases, but that shouldn't be important.)
- Period.ToString() (i.e. NodaTime.Text.PeriodPattern) will therefore have to 
stop reading the Units, and omit zero-valued properties.
- This also means that PeriodBuilder's properties become non-nullable.

Original comment by malcolm.rowe on 26 Jul 2012 at 1:39

  • Changed title: Remove the concept of missing vs zero values from Period.
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

This issue was closed by revision 4d4d39e57456.

Original comment by malcolm.rowe on 27 Jul 2012 at 1:48

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Original comment by malcolm.rowe on 30 Jul 2012 at 7:26

  • Added labels: Milestone-1.0
  • Removed labels: V1-Blocker
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Original comment by malcolm.rowe on 10 Nov 2012 at 10:20

  • Added labels: Milestone-1.0.0
  • Removed labels: Milestone-1.0

@malcolmr malcolmr added the bug label Mar 15, 2015

@malcolmr malcolmr modified the milestone: 1.0.0 Mar 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment