Re: [commons-lang] branch master updated: [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
yes. You are right. Closed
On Mon, 3 Jul 2023 at 10:12, Gilles Sadowski wrote:
> Le lun. 3 juil. 2023 à 09:41, Alex Herbert a
> écrit :
> >
> > On Mon, 3 Jul 2023 at 08:29, sebb wrote:
> > >
> > > Is null checked or unchecked?
> > >
> > > I think neither, so isUnchecked also needs to check for null.
> > >
> > > I wonder whether it might be better to throw NPE in both cases for
> null.
> > >
> > > It may be confusing for users if not checked != unchecked.
> > > e.g. it is tempting to code:
> > >
> > > if (isChecked(t)) {
> > > } else { // must be unChecked
> > > }
> > >
> > > If we don’t throw NPE, then it needs to be made very clear that
> > > isChecked and isUnchecked are not opposites, there is a 3rd case.
> > >
> > > In any case, there needs to be a unit-test specifically for null.
> > >
> > > Sebb
> >
> > +1
> >
> > I reiterate what I originally said, this is missing:
> >
> > @Test
> > public void testIsUnchecked_null() {
> > assertFalse(ExceptionUtils.isUnchecked(null));
> > }
> >
> > The method implementation details are secondary to the fact that the
> > code is not clear on how it handles null; the relationship between
> > isChecked and isUnchecked; and the intended usage.
> >
> > Note that one possible usage of type determination is to decide if you
> > can cast it to a certain type. Currently this method does not provide
> > this functionality as isUnchecked does not ensure that a cast to
> > RuntimeException is allowed (since it passes for Error). So my use
> > case is satisfied by instanceof. This leaves me wondering what are the
> > use cases for isChecked or isUnchecked. I presume you are in an
> > exception block with a Throwable of unknown type. So why do you care
> > if the exception is not checked if not for a recast?
> >
> > Without a use case not satisfied by instanceof then this is code bloat.
> >
>
> Reading through the discussion, I was wondering the same (What is
> the use case?) and was tempted to reach the same conclusion.
> After all, isn't "try/catch" the standard way to tailor behaviour according
> to the exception type?
>
> Regards,
> Gilles
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
Re: [commons-lang] branch master updated: [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
Le lun. 3 juil. 2023 à 09:41, Alex Herbert a écrit :
>
> On Mon, 3 Jul 2023 at 08:29, sebb wrote:
> >
> > Is null checked or unchecked?
> >
> > I think neither, so isUnchecked also needs to check for null.
> >
> > I wonder whether it might be better to throw NPE in both cases for null.
> >
> > It may be confusing for users if not checked != unchecked.
> > e.g. it is tempting to code:
> >
> > if (isChecked(t)) {
> > } else { // must be unChecked
> > }
> >
> > If we don’t throw NPE, then it needs to be made very clear that
> > isChecked and isUnchecked are not opposites, there is a 3rd case.
> >
> > In any case, there needs to be a unit-test specifically for null.
> >
> > Sebb
>
> +1
>
> I reiterate what I originally said, this is missing:
>
> @Test
> public void testIsUnchecked_null() {
> assertFalse(ExceptionUtils.isUnchecked(null));
> }
>
> The method implementation details are secondary to the fact that the
> code is not clear on how it handles null; the relationship between
> isChecked and isUnchecked; and the intended usage.
>
> Note that one possible usage of type determination is to decide if you
> can cast it to a certain type. Currently this method does not provide
> this functionality as isUnchecked does not ensure that a cast to
> RuntimeException is allowed (since it passes for Error). So my use
> case is satisfied by instanceof. This leaves me wondering what are the
> use cases for isChecked or isUnchecked. I presume you are in an
> exception block with a Throwable of unknown type. So why do you care
> if the exception is not checked if not for a recast?
>
> Without a use case not satisfied by instanceof then this is code bloat.
>
Reading through the discussion, I was wondering the same (What is
the use case?) and was tempted to reach the same conclusion.
After all, isn't "try/catch" the standard way to tailor behaviour according
to the exception type?
Regards,
Gilles
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [commons-lang] branch master updated: [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
On Mon, 3 Jul 2023 at 08:29, sebb wrote:
>
> Is null checked or unchecked?
>
> I think neither, so isUnchecked also needs to check for null.
>
> I wonder whether it might be better to throw NPE in both cases for null.
>
> It may be confusing for users if not checked != unchecked.
> e.g. it is tempting to code:
>
> if (isChecked(t)) {
> } else { // must be unChecked
> }
>
> If we don’t throw NPE, then it needs to be made very clear that
> isChecked and isUnchecked are not opposites, there is a 3rd case.
>
> In any case, there needs to be a unit-test specifically for null.
>
> Sebb
+1
I reiterate what I originally said, this is missing:
@Test
public void testIsUnchecked_null() {
assertFalse(ExceptionUtils.isUnchecked(null));
}
The method implementation details are secondary to the fact that the
code is not clear on how it handles null; the relationship between
isChecked and isUnchecked; and the intended usage.
Note that one possible usage of type determination is to decide if you
can cast it to a certain type. Currently this method does not provide
this functionality as isUnchecked does not ensure that a cast to
RuntimeException is allowed (since it passes for Error). So my use
case is satisfied by instanceof. This leaves me wondering what are the
use cases for isChecked or isUnchecked. I presume you are in an
exception block with a Throwable of unknown type. So why do you care
if the exception is not checked if not for a recast?
Without a use case not satisfied by instanceof then this is code bloat.
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [commons-lang] branch master updated: [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
Is null checked or unchecked?
I think neither, so isUnchecked also needs to check for null.
I wonder whether it might be better to throw NPE in both cases for null.
It may be confusing for users if not checked != unchecked.
e.g. it is tempting to code:
if (isChecked(t)) {
} else { // must be unChecked
}
If we don’t throw NPE, then it needs to be made very clear that
isChecked and isUnchecked are not opposites, there is a 3rd case.
In any case, there needs to be a unit-test specifically for null.
Sebb
On Mon, 3 Jul 2023 at 01:29, Elliotte Rusty Harold wrote:
>
> On Mon, Jul 3, 2023 at 12:20 AM Gary Gregory wrote:
> >
> > Hi Elliotte:
> >
> > Might you be looking at some old code in the PR?
> >
>
> Just looking at what was posted in the email thread. It's a weird
> corner case not everyone thinks of.
>
> > The current code is:
> >
> > /**
> > * Checks if a throwable represents a checked exception
> > *
> > * @param throwable
> > *The throwable to check.
> > * @return True if the given Throwable is a checked exception.
> > * @since 3.13.0
> > */
> > public static boolean isChecked(final Throwable throwable) {
> > return throwable != null && !(throwable instanceof Error) &&
> > !(throwable instanceof RuntimeException);
> > }
>
> This also looks wrong. This might work:
>
> public static boolean isChecked(final Throwable throwable) {
> return throwable != null && throwable instanceof Exception &&
> !(throwable instanceof RuntimeException);
> }
>
>
>
> --
> Elliotte Rusty Harold
> elh...@ibiblio.org
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [commons-lang] branch master updated: [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
On Mon, Jul 3, 2023 at 12:20 AM Gary Gregory wrote:
>
> Hi Elliotte:
>
> Might you be looking at some old code in the PR?
>
Just looking at what was posted in the email thread. It's a weird
corner case not everyone thinks of.
> The current code is:
>
> /**
> * Checks if a throwable represents a checked exception
> *
> * @param throwable
> *The throwable to check.
> * @return True if the given Throwable is a checked exception.
> * @since 3.13.0
> */
> public static boolean isChecked(final Throwable throwable) {
> return throwable != null && !(throwable instanceof Error) &&
> !(throwable instanceof RuntimeException);
> }
This also looks wrong. This might work:
public static boolean isChecked(final Throwable throwable) {
return throwable != null && throwable instanceof Exception &&
!(throwable instanceof RuntimeException);
}
--
Elliotte Rusty Harold
elh...@ibiblio.org
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [commons-lang] branch master updated: [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
I just noticed that the line
return !isChecked(throwable)
Means that if throwable is null then it will be considered unchecked. I
will fix that tomorrow by doing
Return throwable ! null && throwable instanceof Exception;
On Mon, 3 Jul 2023, 01:20 Gary Gregory, wrote:
> Hi Elliotte:
>
> Might you be looking at some old code in the PR?
>
> The current code is:
>
> /**
> * Checks if a throwable represents a checked exception
> *
> * @param throwable
> *The throwable to check.
> * @return True if the given Throwable is a checked exception.
> * @since 3.13.0
> */
> public static boolean isChecked(final Throwable throwable) {
> return throwable != null && !(throwable instanceof Error) &&
> !(throwable instanceof RuntimeException);
> }
>
> /**
> * Checks if a throwable represents an unchecked exception
> *
> * @param throwable
> *The throwable to check.
> * @return True if the given Throwable is an unchecked exception.
> * @since 3.13.0
> */
> public static boolean isUnchecked(final Throwable throwable) {
> return !isChecked(throwable);
> }
>
> Gary
>
> On Sun, Jul 2, 2023 at 8:01 PM Elliotte Rusty Harold
> wrote:
> >
> > On Sun, Jul 2, 2023 at 8:53 PM Alex Herbert
> wrote:
> >
> > > public static boolean isUnchecked(final Throwable throwable) {
> > > return (throwable instanceof Error) || (throwable instanceof
> > > RuntimeException);
> > > }
> >
> > Not quite. It's also possible for someone to define a subclass of
> > Throwable directly that is neither an Exception nor an Error.
> >
> >
> > --
> > Elliotte Rusty Harold
> > elh...@ibiblio.org
> >
> > -
> > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> > For additional commands, e-mail: dev-h...@commons.apache.org
> >
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
Re: [commons-lang] branch master updated: [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
Hi Elliotte. I never thought of that, but I don't think it is Apache's
problem if people exit the java convention
On Mon, 3 Jul 2023, 01:02 Elliotte Rusty Harold, wrote:
> On Sun, Jul 2, 2023 at 8:53 PM Alex Herbert
> wrote:
>
> > public static boolean isUnchecked(final Throwable throwable) {
> > return (throwable instanceof Error) || (throwable instanceof
> > RuntimeException);
> > }
>
> Not quite. It's also possible for someone to define a subclass of
> Throwable directly that is neither an Exception nor an Error.
>
>
> --
> Elliotte Rusty Harold
> elh...@ibiblio.org
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
Re: [commons-lang] branch master updated: [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
Hi Elliotte:
Might you be looking at some old code in the PR?
The current code is:
/**
* Checks if a throwable represents a checked exception
*
* @param throwable
*The throwable to check.
* @return True if the given Throwable is a checked exception.
* @since 3.13.0
*/
public static boolean isChecked(final Throwable throwable) {
return throwable != null && !(throwable instanceof Error) &&
!(throwable instanceof RuntimeException);
}
/**
* Checks if a throwable represents an unchecked exception
*
* @param throwable
*The throwable to check.
* @return True if the given Throwable is an unchecked exception.
* @since 3.13.0
*/
public static boolean isUnchecked(final Throwable throwable) {
return !isChecked(throwable);
}
Gary
On Sun, Jul 2, 2023 at 8:01 PM Elliotte Rusty Harold wrote:
>
> On Sun, Jul 2, 2023 at 8:53 PM Alex Herbert wrote:
>
> > public static boolean isUnchecked(final Throwable throwable) {
> > return (throwable instanceof Error) || (throwable instanceof
> > RuntimeException);
> > }
>
> Not quite. It's also possible for someone to define a subclass of
> Throwable directly that is neither an Exception nor an Error.
>
>
> --
> Elliotte Rusty Harold
> elh...@ibiblio.org
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [commons-lang] branch master updated: [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
On Sun, Jul 2, 2023 at 8:53 PM Alex Herbert wrote:
> public static boolean isUnchecked(final Throwable throwable) {
> return (throwable instanceof Error) || (throwable instanceof
> RuntimeException);
> }
Not quite. It's also possible for someone to define a subclass of
Throwable directly that is neither an Exception nor an Error.
--
Elliotte Rusty Harold
elh...@ibiblio.org
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [commons-lang] branch master updated: [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
This change is not null-safe for isUnchecked. This case is missing
from the tests (and currently fails):
@Test
public void testIsUnchecked_null() {
assertFalse(ExceptionUtils.isUnchecked(null));
}
The case fails because it simply negates isChecked. Both methods
should return false when passed a null. Since instanceof handles null
then the isUnchecked method can be updated to:
public static boolean isUnchecked(final Throwable throwable) {
return (throwable instanceof Error) || (throwable instanceof
RuntimeException);
}
This implementation passes all current tests plus the extra one above.
On Sun, 2 Jul 2023 at 20:55, wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> ggregory pushed a commit to branch master
> in repository https://gitbox.apache.org/repos/asf/commons-lang.git
>
>
> The following commit(s) were added to refs/heads/master by this push:
> new 98ef0a413 [LANG-1647] Add and ExceptionUtils.isChecked() and
> isUnchecked() #1069
> 98ef0a413 is described below
>
> commit 98ef0a4138ac032923c4fb12a97b388bde354668
> Author: Gary Gregory
> AuthorDate: Sun Jul 2 15:55:12 2023 -0400
>
> [LANG-1647] Add and ExceptionUtils.isChecked() and isUnchecked() #1069
> ---
> src/changes/changes.xml| 2 +
> .../java/org/apache/commons/lang3/Functions.java | 8 +-
> .../commons/lang3/concurrent/ConcurrentUtils.java | 21 +---
> .../apache/commons/lang3/concurrent/Memoizer.java | 10 +-
> .../commons/lang3/exception/ExceptionUtils.java| 119
> -
> .../apache/commons/lang3/function/Failable.java| 8 +-
> 6 files changed, 77 insertions(+), 91 deletions(-)
>
> diff --git a/src/changes/changes.xml b/src/changes/changes.xml
> index 98a831c53..d4e0e4210 100644
> --- a/src/changes/changes.xml
> +++ b/src/changes/changes.xml
> @@ -208,6 +208,8 @@ The type attribute can be add,update,fix,remove.
> Add Pair.accept(FailableBiConsumer).
> Add Pair.apply(FailableBiFunction).
> Add ReflectionDiffBuilder.setExcludeFieldNames(...)
> and DiffExclude a… #838.
> +Add and ExceptionUtils.isChecked()
> and isUnchecked() #1069
> +Add and use ExceptionUtils.throwUnchecked(throwable).
>
> due-to="Dependabot, XenoAmess, Gary Gregory">Bump actions/cache from 2.1.4 to
> 3.0.10 #742, #752, #764, #833, #867, #959, #964.
> due-to="Dependabot, Gary Gregory">Bump actions/checkout from 2 to 3.1.0 #819,
> #825, #859, #963.
> diff --git a/src/main/java/org/apache/commons/lang3/Functions.java
> b/src/main/java/org/apache/commons/lang3/Functions.java
> index e5e4e106c..7e0de8ba4 100644
> --- a/src/main/java/org/apache/commons/lang3/Functions.java
> +++ b/src/main/java/org/apache/commons/lang3/Functions.java
> @@ -33,6 +33,7 @@ import java.util.function.Supplier;
> import java.util.stream.Stream;
>
> import org.apache.commons.lang3.Streams.FailableStream;
> +import org.apache.commons.lang3.exception.ExceptionUtils;
> import org.apache.commons.lang3.function.Failable;
> import org.apache.commons.lang3.function.FailableBooleanSupplier;
>
> @@ -521,12 +522,7 @@ public class Functions {
> */
> public static RuntimeException rethrow(final Throwable throwable) {
> Objects.requireNonNull(throwable, "throwable");
> -if (throwable instanceof RuntimeException) {
> -throw (RuntimeException) throwable;
> -}
> -if (throwable instanceof Error) {
> -throw (Error) throwable;
> -}
> +ExceptionUtils.throwUnchecked(throwable);
> if (throwable instanceof IOException) {
> throw new UncheckedIOException((IOException) throwable);
> }
> diff --git
> a/src/main/java/org/apache/commons/lang3/concurrent/ConcurrentUtils.java
> b/src/main/java/org/apache/commons/lang3/concurrent/ConcurrentUtils.java
> index bafbad67d..42b6343da 100644
> --- a/src/main/java/org/apache/commons/lang3/concurrent/ConcurrentUtils.java
> +++ b/src/main/java/org/apache/commons/lang3/concurrent/ConcurrentUtils.java
> @@ -61,8 +61,7 @@ public class ConcurrentUtils {
> if (ex == null || ex.getCause() == null) {
> return null;
> }
> -
> -throwCause(ex);
> +ExceptionUtils.throwUnchecked(ex.getCause());
> return new ConcurrentException(ex.getMessage(), ex.getCause());
> }
>
> @@ -84,7 +83,7 @@ public class ConcurrentUtils {
> return null;
> }
>
> -throwCause(ex);
> +ExceptionUtils.throwUnchecked(ex.getCause());
> return new ConcurrentRuntimeException(ex.getMessage(),
> ex.getCause());
> }
>
> @@ -145,22 +144,6 @@ public class ConcurrentUtils {
> return ex;
> }
>
> -/**
> - * Tests whether the cause of the specified {@link ExecutionException}
> - * should be thrown and does it if necessary.
> - *
> - * @param ex the exception in question
> - */
> -
