findepi commented on code in PR #12882:
URL: https://github.com/apache/datafusion/pull/12882#discussion_r1797438908
##########
datafusion/expr/src/utils.rs:
##########
@@ -672,7 +672,7 @@ where
let mut err = Ok(());
expr.apply(|expr| {
if let Err(e) = f(expr) {
- // save the error for later (it may not be a DataFusionError
+ // Save the error for later (it may not be a DataFusionError
Review Comment:
For docs (`///`) casing is important.
For inline comments, i would be more relaxed (but semantics matters a ton!).
But here it would be good to fix lack of closing brace
##########
datafusion/expr/src/utils.rs:
##########
@@ -653,13 +653,13 @@ where
if !(exprs.contains(expr)) {
exprs.push(expr.clone())
}
- // stop recursing down this expr once we find a match
+ // Stop recursing down this expr once we find a match
return Ok(TreeNodeRecursion::Jump);
}
Ok(TreeNodeRecursion::Continue)
})
- // pre_visit always returns OK, so this will always too
+ // Pre_visit always returns OK, so this will always too
Review Comment:
This looks like a verbatim name, so maybe keep it as is?
"... so this will always return OK too"
--
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]
