-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
Allow disabling native type ids when deserializing for #270 #271
Allow disabling native type ids when deserializing for #270 #271
Conversation
78e619e
to
9d18aa3
Compare
* | ||
* @see <a href="https://amzn.github.io/ion-docs/docs/spec.html#annot">The Ion Specification</a> | ||
* | ||
* @since 2.12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If added at this point, must be marked as since 2.12.3 (but I'll add another note on this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I will update it.
Sounds reasonable. The only concern I have is that ideally new functionality should be in a new minor version (2.13). Would like to have another opinion from other contributors, but makes sense to me. I'll add this to my todo list to re-review and make sure it can be merge before 2.12.3 is finalized (there are a few other changes and I plan to release that within 2 weeks or so). |
@manaigrn-amzn quick note: I assume this is under amazon ccla, so no need for separate cla. |
Yeah, it would be greatly appreciated to get this change released as fast as possible. We are on a tight deadline to migrate to 2.12.x, and we are blocked without this change.
Yes, that is correct. |
9d18aa3
to
db8bd30
Compare
} | ||
|
||
IonParser(IonReader r, IonSystem system, IOContext ctxt, ObjectCodec codec) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should retain this intermediate constructor (at least for 2.12.x); will add a deprecated variant after merge.
This allows the check for native type ids to be disabled while deserializing. I would like to get feedback on this approach.
With the following example, native type ids will not be expected when deserializing.
The full explanation of the problem this is solving can be found in #270