stevedlawrence commented on code in PR #768:
URL: https://github.com/apache/daffodil/pull/768#discussion_r857580859
##########
daffodil-io/src/main/scala/org/apache/daffodil/processors/charset/IBM037.scala:
##########
@@ -51,7 +51,26 @@ class BitsCharsetDecoderIBM037
protected override def decodeOneChar(dis: InputSourceDataInputStream, finfo:
FormatInfo): Char = {
val byte = getByte(dis, 0)
- val dec = BitsCharsetIBM037.decodeStringTable(byte)
- dec
+ BitsCharsetIBM037.decodeStringTable(byte)
+ }
+}
+
+final class BitsCharsetIBM037Definition
+ extends BitsCharsetDefinition {
+
+ override def name() = "IBM037"
+
+ override def newInstance() = {
+ BitsCharsetIBM037
+ }
+}
+
+final class BitsCharsetIBM037AliasDefinition
+ extends BitsCharsetDefinition {
Review Comment:
I was thinking about how to reduce much of this boilerplate and also how to
handle aliases and the upper case issue. I like the idea of have separate
Definition classes for aliases, which I didn't notice when I previously
mentioned aliases, but what if we make `BitsCharsetDefinition` something like
this:
```scala
abstract class BitsCharsetDefinition(charset: BitsCharset, alias:
Option[String] = None) {
final def charset(): BitsCharset = charset
final def name(): String = alias.getOrElse(charset.name).toUpperCase()
}
```
So the name is taken from the actual `BitsCharset` if there's no alias, but
has a way to override that with an optional alias parameter.
Then the definitions become less verbose one-liners like this:
```scala
final class BitsCharsetIBM037Definition extends
BitsCharsetDefinition(BitsCharsetIBM037)
final class BitsCharsetEBCDIC_CP_USDefinition extends
BitsCharsetDefinition(BitsCharsetIBM037, Some("EBCDIC-CP-US"))
```
Note that this replaces the `newInstance` function with `charset` because a
BitsCharsetDefintion now can't ever actually return a new instance--it always
returns whatever instance was passed to the constructor. But this is probably
good. We shouldn't make it possible to create more than one instance of
BitsCharset per charset. There should only ever be one stateless thread-safe
`BitsCharset` instance per charset. It is the decoders/encoders created from
those that can have state, and Daffodil will create new instances of those as
needed.
Also note that we can remove the `alias` member from BitsCharset, since
those are all now handled with unique Definition classes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
