lostluck commented on a change in pull request #16776:
URL: https://github.com/apache/beam/pull/16776#discussion_r802125401
##########
File path: sdks/go/pkg/beam/core/typex/class.go
##########
@@ -91,16 +92,25 @@ func ClassOf(t reflect.Type) Class {
// data must be fully serializable. Functions and channels are examples of
invalid
// types. Aggregate types with no universals are considered concrete here.
func IsConcrete(t reflect.Type) bool {
+ concrete, _ := isConcrete(t, make(map[uintptr]bool))
+ return concrete
+}
+
+// AssertConcrete returns true iff the given type is a valid "concrete" data
type and if not,
+// returns false along with an error indicating why the type is not concrete.
Concrete
+// data must be fully serializable. Functions and channels are examples of
invalid
+// types. Aggregate types with no universals are considered concrete here.
+func AssertConcrete(t reflect.Type) (bool, error) {
return isConcrete(t, make(map[uintptr]bool))
}
-func isConcrete(t reflect.Type, visited map[uintptr]bool) bool {
+func isConcrete(t reflect.Type, visited map[uintptr]bool) (bool, error) {
Review comment:
Consider instead only returning the error. In all non-nil errors, we are
returning false, so we might as well avoid the extra bool, then we check
nilness in the IsConcrete variant to maintain the boolean there.
##########
File path: sdks/go/pkg/beam/core/typex/class.go
##########
@@ -91,16 +92,25 @@ func ClassOf(t reflect.Type) Class {
// data must be fully serializable. Functions and channels are examples of
invalid
// types. Aggregate types with no universals are considered concrete here.
func IsConcrete(t reflect.Type) bool {
+ concrete, _ := isConcrete(t, make(map[uintptr]bool))
+ return concrete
+}
+
+// AssertConcrete returns true iff the given type is a valid "concrete" data
type and if not,
+// returns false along with an error indicating why the type is not concrete.
Concrete
+// data must be fully serializable. Functions and channels are examples of
invalid
+// types. Aggregate types with no universals are considered concrete here.
+func AssertConcrete(t reflect.Type) (bool, error) {
Review comment:
Assert generally implies that something will panic or exit, rather than
return an error. Perhaps `CheckConcrete` instead?
--
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]
