uros-b commented on PR #57927:
URL: https://github.com/apache/spark/pull/57927#issuecomment-5302532499
Thank you @LuciferYang for the review and the full grep survey. To clarify a
bit on the intent, it's not only about readability here. For a bool ret call,
ScalaTest's assert macro only reconstructs the expression for the negated form.
For example:
- `assert(securityManager.checkUIViewPermissions("user3") === false)` gives
only "true did not equal false"
- `assert(!securityManager.checkUIViewPermissions("user3"))` gives better
"securityManager.checkUIViewPermissions(\"user3\") was true"
So in a typical suite with many similar permission checks, the negated form
names the offending call in the message itself rather than reporting an
anonymous and vague `true did not equal false`.
That said, both your points make sense to me. I think that a repo wide sweep
does carry risk, and wouldn't go for it. However, I would still suggest to move
forward with this scoped fix at this time and improve at least this part of the
code, e.g. complete conversion of `SecurityManagerSuite` and leave the rest of
the tree alone.
In case you still feel strongly against it, I would be happy to close this
PR as well. Please let me know!
--
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]
