mbeckerle commented on code in PR #1455:
URL: https://github.com/apache/daffodil/pull/1455#discussion_r1993598627
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/AnnotatedSchemaComponent.scala:
##########
@@ -294,7 +294,7 @@ trait AnnotatedSchemaComponent
with AnnotatedMixin
with OverlapCheckMixin {
- protected override def initialize(): Unit = {
+ protected[dsom] override def initialize(): Unit = {
Review Comment:
So an instance of Cat cannot call speak() on a different instance of Cat,
only on itself? I guess I understand why they changed that,.....
adding the package '[dom]' qualification used to add a further restriction
on who could call it, but it sounds like it weakens the access protections now
to allow the cross-instance calls to protected methods. Do I have that right?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -179,7 +179,7 @@ trait ElementBaseGrammarMixin
prefixLengthType
)
schemaDefinitionWhen(
- detachedElementDecl.alignment != 1,
+ detachedElementDecl.alignment.asInstanceOf[Int] != 1,
Review Comment:
Scala 3 has disjunction and union types which in theory allow cleaner
solutions to this sort of thing. Worth learning about in the future.
For this change set I'm ok with just the cast here.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ComplexTypes.scala:
##########
@@ -35,7 +35,7 @@ sealed abstract class ComplexTypeBase(xmlArg: Node,
parentArg: SchemaComponent)
final override def optUnion = None
final override def typeNode = NodeInfo.Complex
- override final def group = modelGroup
+ override final def group: ModelGroup = modelGroup
Review Comment:
Is this a style suggestion, or a requirement? If this is in some way
suppressible we should turn this requirement off.
The name of the value 'modelGroup' was chosen to make its type clear. That's
the reason many parameters and members have names isomorphic to type names, so
that when there is only one role for the type in a piece of code, (which is
very very common), the type information does not have to clutter things up.
This ability to not provide the type names when they are obvious is a big
reason why I like Scala, and why I hated Java, and is why many people like
runtime-typed languages like Lisp and Python.
But it is actually worse than that. When I write 'override final def group =
modelGroup' I actually intend something different than if I explicitly put the
type ModelGroup in the method declaration. It is an intentionally weaker
statement. It allows the type of the base-class version of 'group' to change,
and so long as that change is a super-type of the type of 'modelGroup', then
this code remains correct. Adding in the rigid type 'ModelGroup' makes this
code more brittle to change through redundancy.
And refactoring of object-oriented type hierarchies like the DSOM types,
e.g., to introduce a new intermediate class between say, ModelGroup and Term,
let's call it GroupLikeTerm,.... that would be a routine kind of refactoring.
Making the 'group' method return a 'GroupLikeTerm' instead of 'ModelGroup'
would be legal, and this code would not have to change.
Scala 3 designers certainly had "Python-envy", adopting even the
indent-based syntax idea... but requiring to be explicit about types in these
cases where it is just clutter or where the flexibility is needed... seems
very inconsistent with the idea that Scala is a more strongly-typed way to
program, but achieves the code-density of typeless languages.
##########
daffodil-io/src/main/scala/org/apache/daffodil/io/processors/charset/BitsCharsetDefinition.scala:
##########
@@ -25,10 +25,10 @@ import
org.apache.daffodil.lib.util.SimpleNamedLoadableService
* daffodil-io/src/main/resources/META-INF/services. name() must return a
fully capitalized string
*/
abstract class BitsCharsetDefinition(
- charset: BitsCharset,
+ _charset: BitsCharset,
alias: Option[String] = None
) extends SimpleNamedLoadableService {
- final def name(): String = alias.getOrElse(charset.name).toUpperCase()
+ final def name(): String = alias.getOrElse(_charset.name).toUpperCase()
- final def charset(): BitsCharset = charset
+ final def charset(): BitsCharset = _charset
Review Comment:
I would use charsetArg as the name of the parameter. Having a member/arg and
a zero-parameter method with the same name.... just seems confusing.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2931,9 +2931,11 @@ case class DFDLTestBitExpr(nameAsParsed: String,
fnQName: RefQName, args: List[E
override lazy val inherentType = NodeInfo.Boolean
override def targetTypeForSubexpression(subexpr: Expression): NodeInfo.Kind
= {
+ val dataValue = data
+ val bitPosValue = bitPos
subexpr match {
- case `data` => NodeInfo.UnsignedByte
- case `bitPos` => NodeInfo.UnsignedByte
+ case `dataValue` => NodeInfo.UnsignedByte
+ case `bitPosValue` => NodeInfo.UnsignedByte
Review Comment:
This seems pretty arbitrary. What motivated this change? The 'data' was
just a member of the local class. You are somehow being required to make it
local to the current expression?
--
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]
