API, Spark 3.3: Remove all usages of deprecated AssertHelpers#10500
Conversation
nastra
left a comment
nastra
left a comment
There was a problem hiding this comment.
This might still be used in older Spark versions that haven't been converted over to using AssertJ assertions
|
you're right, just intellij didn't see them. |
findepi
changed the title
findepi
force-pushed
the
findepi/ah
branch
2 times, most recently
from
2c1a684 to
d7069b8
Compare
| scalarSql( | ||
| "CALL %s.system.add_files('%s', '`parquet`.`%s`')", | ||
| catalogName, tableName, fileTableDir.getAbsolutePath())); | ||
| Assertions.assertThatThrownBy( |
There was a problem hiding this comment.
it should be fine to statically import the method here, similar to how assertThat is already statically imported in this class.
There was a problem hiding this comment.
The code was changed mechanically, so thanks for catching that.
I know that static imports are generally discouraged in the project, so is this comment about this source file in particular, or more broadly about Assertions.*?
There was a problem hiding this comment.
the comment is only about statically importing methods from Assertions, but I guess it would be too much work now to statically import assertThatThrownBy, so let's leave it as-is
There was a problem hiding this comment.
If we have consensu that Assertions should be used with static imports, do you think it could be helpful to support this with static code analysis?
There was a problem hiding this comment.
we could eventually support this via static code analysis but atm we have a mix of static vs non-static usage wrt Assertions
There was a problem hiding this comment.
so if we cleaned up the non-static imports, we would want to have static code analysis?
let's give it a try -- #10517
|
flink-scala-2-12-tests (8, 1.17) looks like a flake. on a subsequent run flink-scala-2-12-tests (8, 1.17) it passed. |
`AssertHelpers` was deprecated and all the usages were removed.
nastra
left a comment
nastra
left a comment
There was a problem hiding this comment.
LGTM, thanks for fixing this
nastra
changed the title
|
thanks for the merge! |
2 participants
No description provided.