mdedetrich commented on code in PR #630:
URL: https://github.com/apache/pekko-http/pull/630#discussion_r1844985809
##########
http-core/src/main/scala/org/apache/pekko/http/javadsl/model/ContentType.scala:
##########
@@ -61,4 +61,10 @@ trait ContentType {
* Returns the charset if this ContentType is non-binary.
*/
def getCharsetOption: Optional[HttpCharset]
+
+ /**
+ * Returns the string representation of this ContentType
+ * @since 1.2.0
+ */
+ def value: String
}
Review Comment:
So fyi, changing it to `.getValue()` is more contrived than initially
anticipated. Naively you would do something like
```scala
def getValue: String = value // value comes from `ValueRenderable`
```
However the `org.apache.pekko.http.javadsl.model.ContentType` has no
visibility for the `value` method within the `ValueRenderable` trait (after all
this is the exact issue we are dealing with). In order to make this work
`org.apache.pekko.http.javadsl.model.ContentType` would have to extend
`ValueRenderable` which would also expose the `value` method since its public
i.e.
```scala
/**
* INTERNAL API
*
* An entity that has a rendered value (like an HttpHeader)
*/
@InternalApi
private[http] trait ValueRenderable extends ToStringRenderable {
def value: String = toString
}
```
which means the trait would both have `value` and `getValue` methods, far
from ideal (note that even though the trait is marked as `private[http]`, this
is only visible to the Scala compiler, Java has no concept of it and hence it
only sees it as a standard interface with a public method).
I think this discussion is revealing of what appears to be an overlooked
design misdecision, which is that we should be having 2 `ValueRenderable`
traits, one for Java api and one for Scala api and only the respective
implementations extending those traits.
I can go ahead and do this change but its obviously going to be more work.
Thankfully `ValueRenderable` trait is marked as internal/private which means I
only need to take care that a public `def value: String = toString` exists for
the Scala api.
Wdyt?
##########
http-core/src/main/scala/org/apache/pekko/http/javadsl/model/ContentType.scala:
##########
@@ -61,4 +61,10 @@ trait ContentType {
* Returns the charset if this ContentType is non-binary.
*/
def getCharsetOption: Optional[HttpCharset]
+
+ /**
+ * Returns the string representation of this ContentType
+ * @since 1.2.0
+ */
+ def value: String
}
Review Comment:
So fyi, changing it to `.getValue()` is more contrived than initially
anticipated. Naively you would do something like
```scala
def getValue: String = value // value method comes from `ValueRenderable`
```
However the `org.apache.pekko.http.javadsl.model.ContentType` has no
visibility for the `value` method within the `ValueRenderable` trait (after all
this is the exact issue we are dealing with). In order to make this work
`org.apache.pekko.http.javadsl.model.ContentType` would have to extend
`ValueRenderable` which would also expose the `value` method since its public
i.e.
```scala
/**
* INTERNAL API
*
* An entity that has a rendered value (like an HttpHeader)
*/
@InternalApi
private[http] trait ValueRenderable extends ToStringRenderable {
def value: String = toString
}
```
which means the trait would both have `value` and `getValue` methods, far
from ideal (note that even though the trait is marked as `private[http]`, this
is only visible to the Scala compiler, Java has no concept of it and hence it
only sees it as a standard interface with a public method).
I think this discussion is revealing of what appears to be an overlooked
design misdecision, which is that we should be having 2 `ValueRenderable`
traits, one for Java api and one for Scala api and only the respective
implementations extending those traits.
I can go ahead and do this change but its obviously going to be more work.
Thankfully `ValueRenderable` trait is marked as internal/private which means I
only need to take care that a public `def value: String = toString` exists for
the Scala api.
Wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@pekko.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@pekko.apache.org
For additional commands, e-mail: notifications-h...@pekko.apache.org
