-
Notifications
You must be signed in to change notification settings - Fork 305
convert middleware jackson to kotlin #359
base: feature/kotlin_conversion
Are you sure you want to change the base?
convert middleware jackson to kotlin #359
Conversation
this new pr should address all the comments in https://github.com/NYTimes/Store/pull/350/files . |
/** | ||
* An implementation of [BufferedSourceAdapter] that uses [ObjectMapper] to convert Java values to JSON. | ||
*/ | ||
class JacksonBufferedSourceAdapter<Parsed> @Inject |
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.
The Java class was also containing a logic that would propagate a thrown exception! Is that deleted in purpose on the Kotlin conversion?
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.
Yes! @tiwiz mentioned failing fast, not sure 100% if that is the desired direction. @digitalbuddha
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.
that would be a breaking change imo, leave the same as before pls
private val parsedType: JavaType | ||
|
||
constructor(jsonFactory: JsonFactory, type: Type) { | ||
objectMapper = ObjectMapper(jsonFactory).registerModule(KotlinModule()) |
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.
Maybe add this.
here as well, to match the @Inject
annotated constructor as well?
|
||
@Inject | ||
constructor(objectMapper: ObjectMapper, type: Type) { | ||
this.objectMapper = objectMapper |
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.
Either remove this.
from here or add it to the other constructors as well :)
@Throws(ParserException::class) | ||
override fun apply(@NonNull bufferedSource: BufferedSource): Parsed { | ||
val inputStream = bufferedSource.inputStream() | ||
try { |
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.
Can be lift to return try { ... }
|
||
@Inject | ||
constructor(objectMapper: ObjectMapper, type: Type) { | ||
this.objectMapper = objectMapper |
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.
Same as before, either remove this.
or add it on the other constructor as well.
*/ | ||
@Experimental | ||
fun <Parsed> createObjectToSourceTransformer(objectMapper: ObjectMapper): ObjectToSourceTransformer<Parsed> { | ||
return ObjectToSourceTransformer(JacksonBufferedSourceAdapter(objectMapper)) |
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.
Can be lift to assignment!
No description provided.