refactor to prefer generics and remove useless types #77
Conversation
| case jsObject: JsObject => jsObjectToResponsePayload(jsObject) | ||
| case json => List(Left(ErrorMessageJson("", "", json.compactPrint, Instant.now))) | ||
| } | ||
|
|
||
| implicit def unmarshallerFrom[A](rf: RootJsonReader[A]): Unmarshaller[String, A] = | ||
| _fromStringUnmarshallerFromByteStringUnmarshaller(sprayJsonByteStringUnmarshaller(rf)) | ||
|
|
||
| private def jsObjectToResponsePayload(json: JsObject): List[Either[ErrorMessageJson, FlightStateJson]] = | ||
| private def jsObjectToResponsePayload[A <: FlightStateJson](json: JsObject): List[Either[ErrorMessageJson, A]] = |
There was a problem hiding this comment.
We're converting to a concrete FlightStateJson why use a generic with a cast?
We'll always return List[Either[ErrorMessageJson, FlightStateJson]]
There was a problem hiding this comment.
If you remove the type parameter from openSkyResponsePayloadJsonFormat, can you write a generic version of collectRightMessagesGraph?
There was a problem hiding this comment.
I don't see any problem but I'll double check as soon as I can
| ).toEither.left.map(ex => ErrorMessageJson("", ex.getMessage, json.compactPrint, Instant.now)) | ||
| } | ||
| } | ||
|
|
||
| private def jsArrayToResponsePayload(json: JsArray): List[Either[ErrorMessageJson, MessageJson]] = | ||
| private def jsArrayToResponsePayload[A <: MessageJson](json: JsArray): List[Either[ErrorMessageJson, A]] = |
There was a problem hiding this comment.
Same as above we don't need to be generic and we can avoid all casts
| @@ -21,8 +21,8 @@ object SideStreamContext { | |||
| new KafkaSinkFactory[MonitoringMessageJson, Key, Monitoring.Value](kafkaConfig.monitoringTopic, producerSettings).sink | |||
| } | |||
|
|
|||
| def invalidSink(kafkaConfig: KafkaConfig)(implicit system: ActorSystem): Sink[MessageJson, Future[Done]] = { | |||
| def invalidSink[A](kafkaConfig: KafkaConfig)(implicit system: ActorSystem): Sink[A, Future[Done]] = { | |||
There was a problem hiding this comment.
this sink is used only for FlightMessageJson could we avoid the A type parameter?
There was a problem hiding this comment.
same as above. I tried but then you bind a type of the corresponding graph to FlightMessageJson everywhere. Did you try yourself?
There was a problem hiding this comment.
maybe we can rewrite the mainGraph with:
val collectInvalidFlight = builder.add(
Flow
.fromFunction[Either[ErrorMessageJson, Message], Option[FlightMessageJson]] {
case Right(x: FlightMessageJson) => Some(x)
case _ => None
}
.collect { case Some(x) => x }
)
or something like that. Since the sink is specific for the flight I think is not completely wrong to keep only the FlightMessageJson in this collectInvalidFlight (new name for collectInvalid).
Probably the real problem here is that we are putting a logic specific for FlightMessageJson in a graph used by all type of messages :(
| @@ -26,20 +26,20 @@ object JsonSupport extends SprayJsonSupport with DefaultJsonProtocol { | |||
| implicit val cityMessageJsonFormat: RootJsonFormat[CityMessageJson] = jsonFormat6(CityMessageJson.apply) | |||
| implicit val flightStatesJsonFormat: RootJsonFormat[FlightStatesJson] = jsonFormat2(FlightStatesJson.apply) | |||
|
|
|||
| val aviationEdgePayloadJsonReader: RootJsonReader[List[Either[ErrorMessageJson, MessageJson]]] = { | |||
| def aviationEdgePayloadJsonReader[A]: RootJsonReader[List[Either[ErrorMessageJson, A]]] = { | |||
There was a problem hiding this comment.
do you think we can add a JsonReader context bound so we also rewrite jsArrayToResponsePayload with a Try(json.convertTo[A])? The only draw back is that probably we have to provide the JsonReader directly when calling runAviationEdgeStream or in the ApiProviderStreamContext. What do you think?
There was a problem hiding this comment.
I see what you mean. We may provide the JsonReader for A and let some spray implicit do the dirty work. Let me check whether some implicits exist.
| case jsArray: JsArray => jsArrayToResponsePayload(jsArray) | ||
| case json => List(Left(ErrorMessageJson("", "", json.compactPrint, Instant.now))) | ||
| } | ||
|
|
||
| val openSkyResponsePayloadJsonFormat: RootJsonReader[List[Either[ErrorMessageJson, MessageJson]]] = { | ||
| def openSkyResponsePayloadJsonFormat[A]: RootJsonReader[List[Either[ErrorMessageJson, A]]] = { |
There was a problem hiding this comment.
same proposal here (JsonReader context bound)
No description provided.