Retrofit IOContext with ErrorReportConfiguration by JooHyukKim · Pull Request #1068 · FasterXML/jackson-core (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits11 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
part of #1066
(blocked by #1067)
### Notes
1. As suggested by original PR 1043, trying to minimize changes by
* Introduce "another" new constructor in 2.16(This will keep tests unchanged)
* Use
defaults() method and not use new constructor in old constructors1. After this PR merges, we can do clean ups by
* retrofitting old constructors with new constructor
* Remove the first new constructor in Jackson 2.16 (one without ErrorReportConfiguration
| @@ -141,6 +150,30 @@ public IOContext(StreamReadConstraints src, StreamWriteConstraints swc, BufferRe |
|---|
| _contentReference = contentRef; |
| _sourceRef = contentRef.getRawContent(); |
| _managedResource = managedResource; |
| _errorReportConfiguration = ErrorReportConfiguration.defaults(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead call:
this(src, swc, br, contentRef, managedResource, ErrorReportConfiguration.defaults();
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned above in Notes.1 as...
Use defaults() method and not use new constructor in old constructors
... to minimize changes (git revision), but maybe I have taken it too literally 😅.
Done. Thank you!
| * |
|---|
| * @since 2.16 |
| */ |
| public ErrorReportConfiguration getErrorReportConfiguration() { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow the New Jackson Naming convention :)
and drop "get", so errorReportConfiguration() (like streamWriteConstraints())
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I notice @pjfanning requested getXxx() accessor, so it was "right way" at first. We are moving to reduce use of "get" in cases where there is no existing convention (for particular class) -- get and set method are to be used for general purpose properties/attributes, but not for configuration.
For time being there is obviously some inconsistency: for 3.0 more of get/set prefixes are being/have been removed. But there's no urgency for 2.x.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, difficult call wrt consistency 🤔 but...
get and set method are to be used for general purpose properties/attributes, but not for configuration.
For this reason, let's go back to original change --one without getXxx()
Looks good, added couple of suggestions. You need to merge/rebase from 2.16, I made some changes hopefully easy enough to merge.
Ok, and then one important next step is to wire JsonFactory.Builder to allow configuration... right now we just get ErrorReportConfiguration defaults.
Ok, and then one important next step is to wire
JsonFactory.Builderto allow configuration... right now we just getErrorReportConfigurationdefaults.
Yes, will be done later today 👍🏻