You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "olabusayoT (via GitHub)" <gi...@apache.org> on 2025/02/13 22:08:38 UTC
[PR] Added parens to functions that override functions with paren [daffodil]
olabusayoT opened a new pull request, #1433:
URL: https://github.com/apache/daffodil/pull/1433
- Map.mapValues -> Map.map(... (K, vmapped))
- fixed some more deprecated autoapplication errors
- fixed nullary method with side effect requiring ()
- fixed Parser[Any] warning of inferred type by making it Parser[Number]. explicitly setting it Parser[Any] still resulted in the warning, which seems to generate false positives (based on online searches).
- fixed functions, which override functions with paren, missing the paren
- make InfoDiffable extend DebugCommand so we can use .name on it. Cannot use .collect due to type erasure of inner traits, so used filter/map instead
- fixed some Symbol literals
- remove unused imports
- explicitly return collection.Seq due to type mismatch error since in 2.12 Seq is collection.Seq but in 2.13 it's immutable.Seq
- replace deprecated toStream with to(LazyList)
DAFFODIL-2152
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT merged PR #1433:
URL: https://github.com/apache/daffodil/pull/1433
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956201036
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SimpleTypes.scala:
##########
@@ -276,17 +276,17 @@ final class EnumerationDef(xml: Node, parentType: SimpleTypeDefBase)
SDE("Invalid data for enumeration: %s", e.getMessage)
}
- lazy val repValuesRaw: Seq[String] = {
+ lazy val repValuesRaw: collection.Seq[String] = {
Review Comment:
This also feels weird that we need to specify collection.Seq instead of just using Seq.
##########
project/OsgiCheck.scala:
##########
@@ -83,7 +83,7 @@ object OsgiCheckPlugin extends AutoPlugin {
// should be a list of one.
val packageOwnersMap = packageOwnerTuples
.groupBy { case (packageName, _) => packageName }
- .mapValues { list => list.map { case (_, packageOwner) => packageOwner } }
+ .map { case (k, list) => k -> list.map { case (_, packageOwner) => packageOwner } }
Review Comment:
Thoughts on changing this to something like:
```val packageOwnersMap = packageOwnerTuples
.groupBy { case (packageName, _) => packageName }
.values.map { list => list.map { case (_, packageOwner) => packageOwner } }
```
I don't have a strong opinion, but I do think it makes it a little clearer that we are really only interested in the values of the map.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956319030
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -125,8 +125,8 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
.groupBy {
_._1
}
- .mapValues {
- _.map(_._2)
+ .map { case (k, v) =>
+ k -> v.map(_._2)
Review Comment:
If you use "(" instead of "{" this can be a one-liner that is equally, or more clear:
```
val eventMap = eventTuples.groupBy(_._1).view.mapValues(_.map(_._2)).toMap
```
If eventTuples is just a transient value, then it could be a MapView, not a map, and then the need to call `.view` would go away.
You also could entirely elminate the need to modify any mapValues calls by this kind of technique:
```
import scala.language.implicitConversions
// Define RichMap to add mapValues method
implicit class RichMap[K, V](val underlying: Map[K, V]) extends AnyVal {
def mapValues[V2](f: V => V2): Map[K, V2] = underlying.view.mapValues(f).toMap
}
```
What I like about that is it would let leave the mapValues calls in place, so we can revisit them. Once we convert them all into unrolled map code with `case (k, v) => ...` they're harder to find syntactically.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -256,15 +256,16 @@ trait ElementBase
// Creates a Map[prefix, Set[NS]]. Duplicate NS's will be removed from the
// Set, since it's a Set
- val bindingsGroupedByPrefix = allBindings.groupBy { _._1 }.mapValues { _.map { _._2 } }
+ val bindingsGroupedByPrefix =
Review Comment:
You do not have to rewrite all these mapValues. I believe you can just change `.mapValues` to `.view.mapValues`. This gives a `MapView`, which behaves like a map, except you have to call `toMap` at the end of the chain of operations at some point, possibly, unless a `MapView` will do the job entirely. I think `MapView` is not serializable.
But I imagine you considered that already and decided to avoid the MapView thing. If so, can you explain why?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/AnnotatedSchemaComponent.scala:
##########
@@ -130,7 +131,7 @@ trait ResolvesScopedProperties extends FindPropertyMixin { self: Term =>
// Important - use of stream here insures we don't lookup
// properties down the chain once we have them here.
//
- val str = sources.toStream.map { _.chainFindProperty(pname) }
+ val str = sources.to(LazyList).map { _.chainFindProperty(pname) }
Review Comment:
I had to look up this `.to(LazyList)` You are passing the object LazyList which obeys Factory pattern. Nice generalization. First thing in Scala 2.13/3.x that adds value. I'm actually a fan of this. :-)
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/ChoiceCombinator.scala:
##########
@@ -236,7 +236,7 @@ case class ChoiceCombinator(ch: ChoiceTermBase, alternatives: Seq[Gram])
}
})
- val dispatchBranchKeyMap = dispatchBranchKeyValueTuples.toMap.mapValues { gram =>
+ val dispatchBranchKeyMap = dispatchBranchKeyValueTuples.toMap.map { case (k, gram) =>
Review Comment:
This feels like a case where the original mapValues was clearer (because I don't have to wonder what k is), and calling `.view.mapValues` and then `.toMap` later would be clearer.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956610636
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SimpleTypes.scala:
##########
@@ -276,17 +276,17 @@ final class EnumerationDef(xml: Node, parentType: SimpleTypeDefBase)
SDE("Invalid data for enumeration: %s", e.getMessage)
}
- lazy val repValuesRaw: Seq[String] = {
+ lazy val repValuesRaw: collection.Seq[String] = {
Review Comment:
That's because in 2.12, Seq is an alias for collection.Seq but in 2.13, it's an alias for immutable.Seq, but map/flatMap etc return collection.Seq resulting in the type mismatch error below. Hence we have to explicitly set it to collection.Seq so 2.13 doesn't error out. (https://docs.scala-lang.org/overviews/core/collections-migration-213.html#option-1-migrate-back-to-scalacollectionseq)
SimpleTypes.scala:284:5: type mismatch;
[error] found : Seq[String] (in scala.collection)
[error] required: Seq[String] (in scala.collection.immutable)
[error] res
[error] ^
[error] one error found
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956248359
##########
project/OsgiCheck.scala:
##########
@@ -83,7 +83,7 @@ object OsgiCheckPlugin extends AutoPlugin {
// should be a list of one.
val packageOwnersMap = packageOwnerTuples
.groupBy { case (packageName, _) => packageName }
- .mapValues { list => list.map { case (_, packageOwner) => packageOwner } }
+ .map { case (k, list) => k -> list.map { case (_, packageOwner) => packageOwner } }
Review Comment:
I probably wouldn't have spotted this if Josh hadn't already highlighted this.
But.... anyway I can't get any intuition about now this map, nor the prior mapValues actually work. I read this as 'it needs to be flatmap', but I expect that's untrue.
How about change k and _ into names of the things in those tuples (even though they are unused) to help me out.
(I'm not sure about that. Might end up cluttered. Maybe just initials like k could be pn, and list could be pnPoPairs. The _ could be pn,)
Please also add the type to packageOwnersMap so I can at least see what this is trying to create.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956248359
##########
project/OsgiCheck.scala:
##########
@@ -83,7 +83,7 @@ object OsgiCheckPlugin extends AutoPlugin {
// should be a list of one.
val packageOwnersMap = packageOwnerTuples
.groupBy { case (packageName, _) => packageName }
- .mapValues { list => list.map { case (_, packageOwner) => packageOwner } }
+ .map { case (k, list) => k -> list.map { case (_, packageOwner) => packageOwner } }
Review Comment:
I probably wouldn't have spotted this if Josh hadn't already highlighted this.
But.... anyway I can't get any intuition about now this map, nor the prior mapValues actually work. I read this as 'it needs to be flatmap', but I expect that's untrue.
How about change k and _ into names of the things in those tuples (even though they are unused) to help me out.
(I'm not sure about that. Might end up cluttered. Maybe just initials like k could be pn, and list could be pnPoPairs. The _ could be pn,)
Please also add the type to packageOwnersMap so I can at least see what this is trying to create.
Or.... and I'm really starting to think it is preferable, add the RichMap conversion so we don't have to touch any of these mapValues calls.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956182450
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/debugger/InteractiveDebugger.scala:
##########
@@ -1663,9 +1663,9 @@ class InteractiveDebugger(
val desc = "display differences since the previous pause in the debugger"
val longDesc = desc
- lazy val infoDiffables = Info.subcommands.collect { case diffable: InfoDiffable =>
- diffable
- }
+ lazy val infoDiffables = Info.subcommands
+ .filter(_.isInstanceOf[InfoDiffable])
+ .map(_.asInstanceOf[InfoDiffable])
Review Comment:
This seems like worse syntax, does scala not support this use of `collect` anymore?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956657885
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/ChoiceCombinator.scala:
##########
@@ -236,7 +236,7 @@ case class ChoiceCombinator(ch: ChoiceTermBase, alternatives: Seq[Gram])
}
})
- val dispatchBranchKeyMap = dispatchBranchKeyValueTuples.toMap.mapValues { gram =>
+ val dispatchBranchKeyMap = dispatchBranchKeyValueTuples.toMap.map { case (k, gram) =>
Review Comment:
Maybe we can use .map (...k -> v.mapped) under the hood, that way we keep .mapValues?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956785962
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -125,8 +125,8 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
.groupBy {
_._1
}
- .mapValues {
- _.map(_._2)
+ .map { case (k, v) =>
+ k -> v.map(_._2)
Review Comment:
I had to move the implicit to the mapValues def, but it works for both 2.12 and 2.13 if I do .map ( k -> f(v)) under the hood
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956616506
##########
project/OsgiCheck.scala:
##########
@@ -83,7 +83,7 @@ object OsgiCheckPlugin extends AutoPlugin {
// should be a list of one.
val packageOwnersMap = packageOwnerTuples
.groupBy { case (packageName, _) => packageName }
- .mapValues { list => list.map { case (_, packageOwner) => packageOwner } }
+ .map { case (k, list) => k -> list.map { case (_, packageOwner) => packageOwner } }
Review Comment:
> Thoughts on changing this to something like:
So .mapValues iterates over values, but returns the entire map back, which is why I went with .map instead of .values.map which doesn't return the entire map.
I'll look into the RichMap conversion
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1969765484
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SimpleTypes.scala:
##########
@@ -276,17 +276,17 @@ final class EnumerationDef(xml: Node, parentType: SimpleTypeDefBase)
SDE("Invalid data for enumeration: %s", e.getMessage)
}
- lazy val repValuesRaw: Seq[String] = {
+ lazy val repValuesRaw: collection.Seq[String] = {
Review Comment:
Hmm, but we use `Seq` all over the place without having to specify .collection vs .immutable. It's not clear what's different about this code?
I think maybe it's a scala-xml issue? Looking at its scaladoc, `Node.attribute` always returns `Option[collection.Seq[NodeSeq]]`, regardless of 2.12 vs 2.13:
[scala-xml_2.12 attribute](https://javadoc.io/static/org.scala-lang.modules/scala-xml_2.12/2.3.0/scala/xml/Node.html#attribute(uri:String,key:String):Option[Seq[scala.xml.Node]])
[scala-xml_2.13 attribute](https://javadoc.io/static/org.scala-lang.modules/scala-xml_2.13/2.3.0/scala/xml/Node.html#attribute(uri:String,key:String):Option[scala.collection.Seq[scala.xml.Node]])
So this is probably where this collection.Seq is coming from.
I imagine what we really want to do is convert that to the kind of Seq preferred by the version of scala we are using? It looks like `collection.toSeq` returns another `collection.Seq` on 2.12 but returns an `immutable.Seq` on 2.13, so it seems Scala already thought of how to handle this. So maybe this kind of code really wants to be:
```scala
val res = optNodes.map(_.toSeq).getOrElse(Seq.empty).flatMap { ... }
```
So the first thing we do is `.toSeq` to get it the right kind of Seq, and then operate on that.
I think this suggests that anywhere `collection.Seq` shows up, we probably want to use the `.toSeq` pattern to convert it to the kind of Seq preferred by the version of scala we're using. Hopefully most libraries do not explicitly return collection.Seq so this would be a fairly rare. I'm guessing it's just scala-xml that does it.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956657035
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -125,8 +125,8 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
.groupBy {
_._1
}
- .mapValues {
- _.map(_._2)
+ .map { case (k, v) =>
+ k -> v.map(_._2)
Review Comment:
Since it uses .view.mapValues under the hood, this won't work for 2.12 I believe
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956606339
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/debugger/InteractiveDebugger.scala:
##########
@@ -1663,9 +1663,9 @@ class InteractiveDebugger(
val desc = "display differences since the previous pause in the debugger"
val longDesc = desc
- lazy val infoDiffables = Info.subcommands.collect { case diffable: InfoDiffable =>
- diffable
- }
+ lazy val infoDiffables = Info.subcommands
+ .filter(_.isInstanceOf[InfoDiffable])
+ .map(_.asInstanceOf[InfoDiffable])
Review Comment:
So you can use collect, but because InfoDiffable is an inner trait, it doesn't know the type and causes this error:
The outer reference in this type test cannot be checked at run time.
[error] lazy val infoDiffables = Info.subcommands.collect { case diffable: InfoDiffable =>
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added parens to functions that override functions with paren [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956645222
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -256,15 +256,16 @@ trait ElementBase
// Creates a Map[prefix, Set[NS]]. Duplicate NS's will be removed from the
// Set, since it's a Set
- val bindingsGroupedByPrefix = allBindings.groupBy { _._1 }.mapValues { _.map { _._2 } }
+ val bindingsGroupedByPrefix =
Review Comment:
view.mapValues.toMap didn't work because it breaks 2.12 with the below error. .map(...k -> vMapped) seems like the best alternative
value mapValues is not a member of scala.collection.IterableView[(String, scala.collection.immutable.Set[(String, org.apache.daffodil.lib.xml.NS)]),scala.collection.immutable.Map[String,scala.collection.immutable.Set[(String, org.apache.daffodil.lib.xml.NS)]]]
[error] allBindings.groupBy { _._1 }.view.mapValues { _.map { _._2 } }.toMap
[error] ^
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org