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

Default LocalDate invalid in non-intuative ways #116

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

Comments

Projects
None yet
2 participants
@GoogleCodeExporter

GoogleCodeExporter commented Mar 15, 2015

If you create a default LocalDate with new LocalDate() or default(LocalDate) or 
by not assigning to a LocalDate field and then compare it to another (i.e. 
date1.Equals(date2)), you will get a null reference exception.

I understand that the default value is an invalid value by design, but it 
should really throw an exception to that effect rather than the null exception.

I'm using 1.0.0-beta2

Original issue reported on code.google.com by WalkerCo...@gmail.com on 5 Oct 2012 at 6:30

@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

The trouble is, we'd have to perform a nullity check on *every single 
operation* that could use the calendar system (the important reference type 
here). If we were going to do that, I think I'd rather just make the calendar 
system default to ISO, and have a usable, predictable value. This is 
fundamentally an issue with value types where the "natural zero" isn't a useful 
value :(

I'm not sure of the right approach, really - I think we need benchmarking to 
work out just how painful it would be to put in the null-coalescing operator to 
fix it all magically...

(We could do the same thing for ZonedDateTime, defaulting to UTC.)

Original comment by jonsk...@google.com on 5 Oct 2012 at 6:43

GoogleCodeExporter commented Mar 15, 2015

The trouble is, we'd have to perform a nullity check on *every single 
operation* that could use the calendar system (the important reference type 
here). If we were going to do that, I think I'd rather just make the calendar 
system default to ISO, and have a usable, predictable value. This is 
fundamentally an issue with value types where the "natural zero" isn't a useful 
value :(

I'm not sure of the right approach, really - I think we need benchmarking to 
work out just how painful it would be to put in the null-coalescing operator to 
fix it all magically...

(We could do the same thing for ZonedDateTime, defaulting to UTC.)

Original comment by jonsk...@google.com on 5 Oct 2012 at 6:43

@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

Would it be free to catch the null reference exception and throw a more 
meaningful exception?

In any case, this doesn't block 1.0, since any interface change will be making 
something that isn't valid into something that is (or that throws a more 
meaningful exception, which I think is fine).  Tentatively putting into 1.0.1.

Original comment by malcolm.rowe on 20 Oct 2012 at 8:52

  • Added labels: Milestone-1.0.1

GoogleCodeExporter commented Mar 15, 2015

Would it be free to catch the null reference exception and throw a more 
meaningful exception?

In any case, this doesn't block 1.0, since any interface change will be making 
something that isn't valid into something that is (or that throws a more 
meaningful exception, which I think is fine).  Tentatively putting into 1.0.1.

Original comment by malcolm.rowe on 20 Oct 2012 at 8:52

  • Added labels: Milestone-1.0.1
@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

Original comment by malcolm.rowe on 25 Oct 2012 at 4:23

  • Changed state: Accepted

GoogleCodeExporter commented Mar 15, 2015

Original comment by malcolm.rowe on 25 Oct 2012 at 4:23

  • Changed state: Accepted
@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

I've benchmarked using the null-coalescing operator for the calendar, which is 
more performance-sensitive than the time zone, and it seems to be okay.

Another contributor has already sent me a patch with all the code changes and 
tests, so I'll add that this evening.

Original comment by jonathan.skeet on 12 Nov 2012 at 9:24

  • Changed state: Started

GoogleCodeExporter commented Mar 15, 2015

I've benchmarked using the null-coalescing operator for the calendar, which is 
more performance-sensitive than the time zone, and it seems to be okay.

Another contributor has already sent me a patch with all the code changes and 
tests, so I'll add that this evening.

Original comment by jonathan.skeet on 12 Nov 2012 at 9:24

  • Changed state: Started
@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

This issue was closed by revision f7a92b2614ca.

Original comment by jonathan.skeet on 12 Nov 2012 at 8:55

  • Changed state: Fixed

GoogleCodeExporter commented Mar 15, 2015

This issue was closed by revision f7a92b2614ca.

Original comment by jonathan.skeet on 12 Nov 2012 at 8:55

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

We actually did this in the 1.1 branch (i.e. default); I don't see a strong 
argument for backporting.

Original comment by malcolm.rowe on 8 Dec 2012 at 5:45

  • Added labels: Milestone-1.1.0
  • Removed labels: Milestone-1.0.1

GoogleCodeExporter commented Mar 15, 2015

We actually did this in the 1.1 branch (i.e. default); I don't see a strong 
argument for backporting.

Original comment by malcolm.rowe on 8 Dec 2012 at 5:45

  • Added labels: Milestone-1.1.0
  • Removed labels: Milestone-1.0.1

@malcolmr malcolmr added the bug label Mar 15, 2015

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

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