Skip to content
This repository has been archived by the owner on Dec 7, 2019. It is now read-only.

convert middleware jackson to kotlin #359

Open
wants to merge 1 commit into
base: feature/kotlin_conversion
Choose a base branch
from

Conversation

brianPlummer
Copy link
Contributor

No description provided.

@brianPlummer
Copy link
Contributor Author

this new pr should address all the comments in https://github.com/NYTimes/Store/pull/350/files .
@digitalbuddha @pavlospt

/**
* An implementation of [BufferedSourceAdapter] that uses [ObjectMapper] to convert Java values to JSON.
*/
class JacksonBufferedSourceAdapter<Parsed> @Inject
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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())
Copy link
Contributor

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
Copy link
Contributor

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 {
Copy link
Contributor

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
Copy link
Contributor

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))
Copy link
Contributor

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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants