ssimeonov commented on a change in pull request #35084:
URL: https://github.com/apache/spark/pull/35084#discussion_r777163351
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala
##########
@@ -810,4 +810,15 @@ class TreeNodeSuite extends SparkFunSuite with SQLHelper {
fail("TreeNode.nodeName should not throw malformed class name error")
}
}
+
+ object Spark37800 {
+ case class TestNode(set: Set[String]) extends LogicalPlan with LeafNode {
Review comment:
In a large codebase, it's important to keep developer mental load to a
minimum. One way to do this is to make the intent of definitions very clear.
If we remove the scoping of `TestNode` there may be ambiguity as to its
purpose. Is it meant to be used just in the one test or does it have a broader
purpose? If more than one test uses this class, it makes more sense for it to
have scope and/or name independent of a specific test. Alas, `FunSuite` in
ScalaTest doesn't create automatic test scopes the way, say, `WordSpec` does.
Anyway, if you prefer `TestNode` being unscoped, I'll be happy to remove
`Spark37800`. In that case, would you be OK with renaming `TestNode` to
`Spark37800Node`?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
