stevedlawrence commented on a change in pull request #768: URL: https://github.com/apache/daffodil/pull/768#discussion_r840841427
########## File path: daffodil-io/src/main/resources/META-INF/services/org.apache.daffodil.processors.charset.BitsCharsetDefinition ########## @@ -0,0 +1,54 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +org.apache.daffodil.processors.charset.BitsCharsetAISPayloadArmoringDefinition +org.apache.daffodil.processors.charset.BitsCharsetBase4LSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetBase4MSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetBinaryLSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetBinaryMSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetHexLSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetHexMSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetIBM037AliasDefinition +org.apache.daffodil.processors.charset.BitsCharsetIBM037Definition +org.apache.daffodil.processors.charset.BitsCharsetISO_8859_1_8BitPackedLSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetISO_8859_1_8BitPackedMSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetISO_8859_1_Definition +org.apache.daffodil.processors.charset.BitsCharsetOctalLSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetOctalMSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetUSASCII6BitPackedLSBFAliasDefinition +org.apache.daffodil.processors.charset.BitsCharsetUSASCII6BitPackedLSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetUSASCII6BitPackedMSBFDefinition +org.apache.daffodil.processors.charset.BitsCharsetUSASCII7BitPackedDefinition +org.apache.daffodil.processors.charset.BitsCharsetUSASCIIAliasDefinition +org.apache.daffodil.processors.charset.BitsCharsetUSASCIIDefinition +org.apache.daffodil.processors.charset.BitsCharsetUTF16BEAliasDefinition +org.apache.daffodil.processors.charset.BitsCharsetUTF16BEDefinition +org.apache.daffodil.processors.charset.BitsCharsetUTF16LEDefinition +org.apache.daffodil.processors.charset.BitsCharsetUTF32BEAliasDefinition +org.apache.daffodil.processors.charset.BitsCharsetUTF32BEDefinition +org.apache.daffodil.processors.charset.BitsCharsetUTF32LEDefinition +org.apache.daffodil.processors.charset.BitsCharsetUTF8Definition +org.apache.daffodil.processors.charset.BitsCharset3BitDFI336DUI001Definition +org.apache.daffodil.processors.charset.BitsCharset3BitDFI746DUI002Definition +org.apache.daffodil.processors.charset.BitsCharset3BitDFI747DUI001Definition +org.apache.daffodil.processors.charset.BitsCharset4BitDFI746DUI002Definition +org.apache.daffodil.processors.charset.BitsCharset5BitDFI1661DUI001Definition +org.apache.daffodil.processors.charset.BitsCharset5BitDFI769DUI002Definition +org.apache.daffodil.processors.charset.BitsCharset5BitPackedLSBFDefinition +org.apache.daffodil.processors.charset.BitsCharset6BitDFI264DUI001Definition +org.apache.daffodil.processors.charset.BitsCharset6BitDFI311DUI002Definition +org.apache.daffodil.processors.charset.BitsCharset6BitICAOAircraftIDAliasDefinition +org.apache.daffodil.processors.charset.BitsCharset6BitICAOAircraftIDDefinition Review comment: Now that we have this capability for schemas to provide their own charset implementations, does it make sense to deprecate some of these that are very specific to certain schema? Perhaps we output a warning when certain charsets are used in a schema and nudge users to move these to some other package? Doesn't have to be done as part of this, but maybe a separate issue/PR? ########## File path: daffodil-io/src/main/scala/org/apache/daffodil/processors/charset/BitsCharsetDefinition.scala ########## @@ -0,0 +1,34 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.daffodil.processors.charset + +/** + * Must be implemented by all charsets. + * + * These are the classes which must be dynamically loaded in order to add a charset implementation + * to Daffodil. All charsets must implement this class + * + * A saved processor does NOT serialize this class. It calls the newFactory method and serializes + * the resulting BitsCharsetFactory. + */ +abstract class BitsCharsetDefinition { + + def name(): String + + def newFactory(): BitsCharsetFactory Review comment: Is there a use case for the Factory indirection? Based on the comment it seems to be related to serialization, but I don't see anywhere where we store the created factories where they could be serialized. The only use I see is `find(...).newFactory().newInstance()`. So we create a factory, create an instance from that factory, and throw away the factory. I wonder if we can simplify this and just treat `BitsCharsetDefinition` as if it were the factory--it can have the `newInstance` function that is directly called when a new instance of a charset is needed? ########## File path: daffodil-io/src/main/scala/org/apache/daffodil/processors/charset/BitsCharsetFactory.scala ########## @@ -0,0 +1,32 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.daffodil.processors.charset + +/** + * Factory for a Daffodil charset. + * + * This is the serialized object which is saved as part of a processor. + * It constructs a charset at runtime when newInstance() is called. Review comment: Is this true anymore?Mentioned in another comment, but I don't see this factory being stored anywhere to be serialized? I wonder if things changed so we only serialized the name of a charset, and on deserialization we look up the charset by that string and create a new instance? -- 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]
