Skip to content
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

Error: Hour 0 must be between 1 and 23 inclusive #65

Closed
kadishmal opened this issue Jul 5, 2017 · 4 comments · Fixed by #68
Closed

Error: Hour 0 must be between 1 and 23 inclusive #65

kadishmal opened this issue Jul 5, 2017 · 4 comments · Fixed by #68

Comments

@kadishmal
Copy link
Contributor

I have the following Ion structure which was properly created in Java:

'[email protected]'::{
  start_date:2017-05-01T00:00:00.000Z,
  end_date:2017-06-01T00:00:00.000Z
}

When reading this data, ion-js fails with the following error message.

Error: Hour 0 must be between 1 and 23 inclusive
    at Timestamp.checkValid (/ion-js/dist/commonjs/es6/IonTimestamp.js:242:27)
    at new Timestamp (/ion-js/dist/commonjs/es6/IonTimestamp.js:220:14)
    at read_timestamp_value (/ion-js/dist/commonjs/es6/IonParserBinaryRaw.js:358:9)
    at ParserBinaryRaw.load_value (/ion-js/dist/commonjs/es6/IonParserBinaryRaw.js:573:21)
    at ParserBinaryRaw.timestampValue (/ion-js/dist/commonjs/es6/IonParserBinaryRaw.js:756:15)
    at BinaryReader.timestampValue (/ion-js/dist/commonjs/es6/IonBinaryReader.js:168:29)
@therapon
Copy link
Contributor

therapon commented Jul 5, 2017

Thanks for the example and stack trace.

It seems that there are a couple of issues with timestamp within ion-js but also with interoperating with othen Ion Libs (e.g., Java).

I'll be looking into building testing infrastructure to round trip Ion and ensure that ion-js can commute with results from and to other Ion Libs.

@kadishmal
Copy link
Contributor Author

I suppose the accepted hours should be between 0 and 23. Don't you think so? If so, I suppose this is just a one line fix + tests. If so, I can push a fix.

Regarding interoperability with other languages, aren't you following the spec? Because spec is spec in any language. https://amzn.github.io/ion-docs/spec.html suggests that 00 is an acceptable value for an hour.

@therapon
Copy link
Contributor

therapon commented Jul 5, 2017

PRs are always welcome :)

You are correct about the Spec and this specific issue. There are other aspects of the spec that allow for implementations to make certain decisions. My previous comment was more in regards to getting all such cases dealt with in a systematic way. Sorry for not expanding a little more on that topic.

@kadishmal
Copy link
Contributor Author

kadishmal commented Jul 5, 2017

The following are from the above-mentioned spec. Some of them fail with the current code.

2007-02-23T12:14Z                // Seconds are optional, but local offset is not
2007-02-23T12:14:33.079-08:00    // A timestamp with millisecond precision and PST local time
2007-02-23T20:14:33.079Z         // The same instant in UTC ("zero" or "zulu")
2007-02-23T20:14:33.079+00:00    // The same instant, with explicit local offset
2007-02-23T20:14:33.079-00:00    // The same instant, with unknown local offset

2007-01-01T00:00-00:00           // Happy New Year in UTC, unknown local offset
2007-01-01                       // The same instant, with days precision, unknown local offset
2007-01-01T                      //    The same value, different syntax.
2007-01T                         // The same instant, with months precision, unknown local offset
2007T                            // The same instant, with years precision, unknown local offset

2007-02-23                       // A day, unknown local offset 
2007-02-23T00:00Z                // The same instant, but more precise and in UTC
2007-02-23T00:00+00:00           // An equivalent format for the same value
2007-02-23T00:00:00-00:00        // The same instant, with seconds precision

2007                             // Not a timestamp, but an int
2007-01                          // ERROR: Must end with 'T' if not whole-day precision, this results as an invalid-numeric-stopper error
2007-02-23T20:14:33.Z            // ERROR: Must have at least one digit precision after decimal point.

therapon pushed a commit that referenced this issue Jul 6, 2017
fix(source): Fix #65: Error: Hour 0 must be between 1 and 23 inclusive.
therapon pushed a commit that referenced this issue Jul 6, 2017
fix(source): Fix #65: Error: Hour 0 must be between 1 and 23 inclusive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants