You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/03/01 17:46:26 UTC
[GitHub] [iceberg] nastra opened a new pull request, #6977: API: Deprecate AssertHelpers
nastra opened a new pull request, #6977:
URL: https://github.com/apache/iceberg/pull/6977
I believe we should point people for newly written code to use `Assertions#assertThatThrownBy(..)` as it provides a more fluent way of asserting exceptions
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6977: API: Deprecate AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6977:
URL: https://github.com/apache/iceberg/pull/6977#discussion_r1129180557
##########
api/src/test/java/org/apache/iceberg/AssertHelpers.java:
##########
@@ -23,7 +23,22 @@
import org.apache.avro.generic.GenericRecord;
import org.assertj.core.api.AbstractThrowableAssert;
import org.assertj.core.api.Assertions;
+import org.assertj.core.api.ThrowableAssert;
+/**
+ * This class is deprecated. Please use {@link
+ * Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly as shown below:
+ *
+ * <pre>
+ * Assertions.assertThatThrownBy(() -> throwingCallable)
+ * .isInstanceOf(ExpectedException.class)
+ * .hasMessage(expectedErrorMsg)
+ * </pre>
+ *
+ * @deprecated Use {@link Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly
+ * as it provides a more fluent way of asserting on exceptions.
Review Comment:
I opened https://github.com/apache/iceberg/issues/7040 to address that
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #6977: API: Deprecate AssertHelpers
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #6977:
URL: https://github.com/apache/iceberg/pull/6977
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Deprecate AssertHelpers [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6977:
URL: https://github.com/apache/iceberg/pull/6977#issuecomment-2170594054
follow-up to complete removal of AssertHelpers -- https://github.com/apache/iceberg/pull/10500
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6977: API: Deprecate AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6977:
URL: https://github.com/apache/iceberg/pull/6977#discussion_r1131457706
##########
api/src/test/java/org/apache/iceberg/AssertHelpers.java:
##########
@@ -23,7 +23,22 @@
import org.apache.avro.generic.GenericRecord;
import org.assertj.core.api.AbstractThrowableAssert;
import org.assertj.core.api.Assertions;
+import org.assertj.core.api.ThrowableAssert;
+/**
+ * This class is deprecated. Please use {@link
+ * Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly as shown below:
+ *
+ * <pre>
+ * Assertions.assertThatThrownBy(() -> throwingCallable)
+ * .isInstanceOf(ExpectedException.class)
+ * .hasMessage(expectedErrorMsg)
+ * </pre>
+ *
+ * @deprecated Use {@link Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly
+ * as it provides a more fluent way of asserting on exceptions.
Review Comment:
good point @jackye1995. I've added deprecations to all methods inside that class
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #6977: API: Deprecate AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #6977:
URL: https://github.com/apache/iceberg/pull/6977#issuecomment-1452388434
> Should we also remove the usage of `AssertHelpers`? Could be a separate PR.
That would be ideal in the long run, but the diff will be quite big, because it's being used at lots of places. At the very least I think we can start and not use it in new tests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #6977: API: Deprecate AssertHelpers
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #6977:
URL: https://github.com/apache/iceberg/pull/6977#discussion_r1125057764
##########
api/src/test/java/org/apache/iceberg/AssertHelpers.java:
##########
@@ -23,7 +23,22 @@
import org.apache.avro.generic.GenericRecord;
import org.assertj.core.api.AbstractThrowableAssert;
import org.assertj.core.api.Assertions;
+import org.assertj.core.api.ThrowableAssert;
+/**
+ * This class is deprecated. Please use {@link
+ * Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly as shown below:
+ *
+ * <pre>
+ * Assertions.assertThatThrownBy(() -> throwingCallable)
+ * .isInstanceOf(ExpectedException.class)
+ * .hasMessage(expectedErrorMsg)
+ * </pre>
+ *
+ * @deprecated Use {@link Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly
+ * as it provides a more fluent way of asserting on exceptions.
Review Comment:
My only issue with this is making sure that `hasMessage` is called. How do we enforce that?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6977: API: Deprecate AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6977:
URL: https://github.com/apache/iceberg/pull/6977#discussion_r1125421211
##########
api/src/test/java/org/apache/iceberg/AssertHelpers.java:
##########
@@ -23,7 +23,22 @@
import org.apache.avro.generic.GenericRecord;
import org.assertj.core.api.AbstractThrowableAssert;
import org.assertj.core.api.Assertions;
+import org.assertj.core.api.ThrowableAssert;
+/**
+ * This class is deprecated. Please use {@link
+ * Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly as shown below:
+ *
+ * <pre>
+ * Assertions.assertThatThrownBy(() -> throwingCallable)
+ * .isInstanceOf(ExpectedException.class)
+ * .hasMessage(expectedErrorMsg)
+ * </pre>
+ *
+ * @deprecated Use {@link Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly
+ * as it provides a more fluent way of asserting on exceptions.
Review Comment:
that's a valid point. I was thinking maybe we could introduce a checkstyle rule (a multiline regex) that would make sure checks against the error message are done. I can open a separate PR for that.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6977: API: Deprecate AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6977:
URL: https://github.com/apache/iceberg/pull/6977#discussion_r1129775705
##########
api/src/test/java/org/apache/iceberg/AssertHelpers.java:
##########
@@ -23,7 +23,22 @@
import org.apache.avro.generic.GenericRecord;
import org.assertj.core.api.AbstractThrowableAssert;
import org.assertj.core.api.Assertions;
+import org.assertj.core.api.ThrowableAssert;
+/**
+ * This class is deprecated. Please use {@link
+ * Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly as shown below:
+ *
+ * <pre>
+ * Assertions.assertThatThrownBy(() -> throwingCallable)
+ * .isInstanceOf(ExpectedException.class)
+ * .hasMessage(expectedErrorMsg)
+ * </pre>
+ *
+ * @deprecated Use {@link Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly
+ * as it provides a more fluent way of asserting on exceptions.
Review Comment:
all calling sites that use any method from `AssertHelpers` will see that the class itself is deprecated, so I don't think we need to deprecate each method individually
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #6977: API: Deprecate AssertHelpers
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6977:
URL: https://github.com/apache/iceberg/pull/6977#discussion_r1131448557
##########
api/src/test/java/org/apache/iceberg/AssertHelpers.java:
##########
@@ -23,7 +23,22 @@
import org.apache.avro.generic.GenericRecord;
import org.assertj.core.api.AbstractThrowableAssert;
import org.assertj.core.api.Assertions;
+import org.assertj.core.api.ThrowableAssert;
+/**
+ * This class is deprecated. Please use {@link
+ * Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly as shown below:
+ *
+ * <pre>
+ * Assertions.assertThatThrownBy(() -> throwingCallable)
+ * .isInstanceOf(ExpectedException.class)
+ * .hasMessage(expectedErrorMsg)
+ * </pre>
+ *
+ * @deprecated Use {@link Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly
+ * as it provides a more fluent way of asserting on exceptions.
Review Comment:
I did some small tests, there is a small difference in IDE: if only the static method is imported the user won't see deprecation warning in code unless he/she looks at the import block, so it might still be beneficial to mark methods as deprecated as well.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra closed pull request #6977: API: Deprecate AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra closed pull request #6977: API: Deprecate AssertHelpers
URL: https://github.com/apache/iceberg/pull/6977
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #6977: API: Deprecate AssertHelpers
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #6977:
URL: https://github.com/apache/iceberg/pull/6977#discussion_r1129731693
##########
api/src/test/java/org/apache/iceberg/AssertHelpers.java:
##########
@@ -23,7 +23,22 @@
import org.apache.avro.generic.GenericRecord;
import org.assertj.core.api.AbstractThrowableAssert;
import org.assertj.core.api.Assertions;
+import org.assertj.core.api.ThrowableAssert;
+/**
+ * This class is deprecated. Please use {@link
+ * Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly as shown below:
+ *
+ * <pre>
+ * Assertions.assertThatThrownBy(() -> throwingCallable)
+ * .isInstanceOf(ExpectedException.class)
+ * .hasMessage(expectedErrorMsg)
+ * </pre>
+ *
+ * @deprecated Use {@link Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly
+ * as it provides a more fluent way of asserting on exceptions.
Review Comment:
Do we need deprecations on the methods as well?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #6977: API: Deprecate AssertHelpers
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #6977:
URL: https://github.com/apache/iceberg/pull/6977#issuecomment-1467097023
@nastra, @jackye1995 I merged this. It will be a good move, but we have to make sure we are catching the message validation in reviews!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org