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

Improve error reporting from pattern parse failures #288

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

Comments

Projects
None yet
2 participants
@GoogleCodeExporter

GoogleCodeExporter commented Mar 15, 2015

(Adapted from 
http://stackoverflow.com/questions/23964736/issue-parsing-nodatime-localdate-fro
m-standard-pattern)

Find the bug in the following:

    var pattern = LocalDatePattern.CreateWithInvariantCulture("D");
    var parseResult = pattern.Parse("Monday, May 26 2014");
    var localDate = parseResult.GetValueOrThrow();
    Console.WriteLine("Parsed value is {0}.", localDate);

The error is:

NodaTime.Text.UnparsableValueException: The value string does not match the 
required number from the format string "dd".

It would be really much more useful if the message explained what the string 
(or at least, string position) was at the point it was trying to find a "dd".

(Tentatively scheduling for 1.3 pending discussion, but if this isn't trivial, 
will punt to later.)

Original issue reported on code.google.com by malcolm.rowe on 31 May 2014 at 9:27

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

We could put it in the message, but that's *slightly* fiddly - it would be 
simpler to add a separate property...

Original comment by jonathan.skeet on 31 May 2014 at 1:25

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

This issue was closed by revision 6c55ba422471.

Original comment by jonathan.skeet on 12 Jun 2014 at 7:39

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

I'm hoping it's fixed, anyway :)

Here are a couple of samples. Basically there are two situations - ones where 
we fail part-way through the parse, and ones where we only fail afterwards, at 
the point where we try to use the values.

Code: LocalDateTimePattern.ExtendedIsoPattern.Parse("1959-XX-12T00:00:00").Value

Unhandled Exception: NodaTime.Text.UnparsableValueException: The value string 
does not match the required number from the format string "MM". Value being 
parsed: '1959-^XX-12T00:00:00'. (^ indicates error position.)


Code: LocalDateTimePattern.ExtendedIsoPattern.Parse("1959-15-12T00:00:00").Value

Unhandled Exception: NodaTime.Text.UnparsableValueException: The month 15 is 
out of range in year 1959. Value being parsed: '1959-15-12T00:00:00'.

Now you could definitely argue that we should bork at 15, and indicate that as 
the error position. However:

- That (in theory) requires knowing which calendar system we're parsing
- ... and that might be specified *after* the month

Options:
- Live with it... at least we're providing the value now!
- Understand the maximum value for months (and days of month) over *all* 
calendar systems
- Remember the position of different values, so we can retrospectively fail 
with the right position
- Use the fact that we know whether or not the pattern has a calendar specifier 
to use a calendar-specific "max month" value early if we know that it's not 
going to change

I'm fine to live with it for the moment, but the last option might make a 
certain amount of sense.

Original comment by jonathan.skeet on 12 Jun 2014 at 7:44

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Original comment by malcolm.rowe on 22 Jun 2014 at 12:15

  • Added labels: Milestone-1.3.0
  • Removed labels: Milestone-1.3-consider
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment