You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2024/11/14 12:13:57 UTC
[PR] There are exceptions tou want to deal with [maven]
cstamas opened a new pull request, #1910:
URL: https://github.com/apache/maven/pull/1910
So make them checked and let compiler help you.
Made checked 3 exceptions:
* ModelBuilderEx
* ProjectBuilderEx
* ModelResolverEx
And it immediately uncovered an avalanche of problems.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Exception usage cleanup [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #1910:
URL: https://github.com/apache/maven/pull/1910#issuecomment-2480677220
Removed all the fluff, now really only the proper placement of exceptions are done (and at least two bugfixes where runtime ex "escaped" causing "internal maven error"). @gnodet pls review
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] There are exceptions you want to deal with [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1910:
URL: https://github.com/apache/maven/pull/1910#discussion_r1842176036
##########
impl/maven-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelBuilder.java:
##########
@@ -1733,12 +1734,50 @@ ModelSource resolveReactorModel(String groupId, String artifactId, String versio
return null;
}
- private <T> T cache(String groupId, String artifactId, String version, String tag, Supplier<T> supplier) {
- return cache.computeIfAbsent(groupId, artifactId, version, tag, supplier);
+ @FunctionalInterface
+ private interface ThrowingSupplier<T, E extends Exception> {
+ T get() throws E;
+ }
+
+ private static class ThrowingSupplierWrapper<T, E extends Exception> implements Supplier<T> {
+ private final ThrowingSupplier<T, E> throwingSupplier;
+
+ private ThrowingSupplierWrapper(ThrowingSupplier<T, E> throwingSupplier) {
+ this.throwingSupplier = throwingSupplier;
+ }
+
+ @Override
+ public T get() {
+ try {
+ return throwingSupplier.get();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static <T, E extends Exception> T process(
+ ThrowingSupplier<T, E> throwingSupplier, Function<Supplier<T>, T> consumer) throws E {
+ try {
+ return consumer.apply(new ThrowingSupplierWrapper<>(throwingSupplier));
+ } catch (RuntimeException e) {
+ if (e.getClass().equals(RuntimeException.class) && e.getCause() != null) {
+ throw (E) e.getCause();
+ } else {
+ throw e;
+ }
+ }
+ }
+ }
+
+ private <T, E extends Exception> T cache(
+ String groupId, String artifactId, String version, String tag, ThrowingSupplier<T, E> supplier)
+ throws E {
+ return ThrowingSupplierWrapper.process(
+ supplier, s -> cache.computeIfAbsent(groupId, artifactId, version, tag, s));
}
Review Comment:
most of this undone, we need to deal with one single exception only
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] There are exceptions you want to deal with [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1910:
URL: https://github.com/apache/maven/pull/1910#discussion_r1842178580
##########
impl/maven-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelBuilder.java:
##########
@@ -1733,12 +1734,50 @@ ModelSource resolveReactorModel(String groupId, String artifactId, String versio
return null;
}
- private <T> T cache(String groupId, String artifactId, String version, String tag, Supplier<T> supplier) {
- return cache.computeIfAbsent(groupId, artifactId, version, tag, supplier);
+ @FunctionalInterface
+ private interface ThrowingSupplier<T, E extends Exception> {
+ T get() throws E;
+ }
+
+ private static class ThrowingSupplierWrapper<T, E extends Exception> implements Supplier<T> {
+ private final ThrowingSupplier<T, E> throwingSupplier;
+
+ private ThrowingSupplierWrapper(ThrowingSupplier<T, E> throwingSupplier) {
+ this.throwingSupplier = throwingSupplier;
+ }
+
+ @Override
+ public T get() {
+ try {
+ return throwingSupplier.get();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static <T, E extends Exception> T process(
+ ThrowingSupplier<T, E> throwingSupplier, Function<Supplier<T>, T> consumer) throws E {
+ try {
+ return consumer.apply(new ThrowingSupplierWrapper<>(throwingSupplier));
+ } catch (RuntimeException e) {
+ if (e.getClass().equals(RuntimeException.class) && e.getCause() != null) {
+ throw (E) e.getCause();
+ } else {
+ throw e;
+ }
+ }
+ }
+ }
+
+ private <T, E extends Exception> T cache(
+ String groupId, String artifactId, String version, String tag, ThrowingSupplier<T, E> supplier)
+ throws E {
+ return ThrowingSupplierWrapper.process(
+ supplier, s -> cache.computeIfAbsent(groupId, artifactId, version, tag, s));
}
Review Comment:
IMO, the latest push is quite sensible. Also, have to add "boilerplate code wanted to avoid" vs all the changes revealed just by the compiler as exception when from unchecked to checked... which one is worth more for you?
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] There are exceptions you want to deal with [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1910:
URL: https://github.com/apache/maven/pull/1910#discussion_r1842143877
##########
impl/maven-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelBuilder.java:
##########
@@ -1733,12 +1734,50 @@ ModelSource resolveReactorModel(String groupId, String artifactId, String versio
return null;
}
- private <T> T cache(String groupId, String artifactId, String version, String tag, Supplier<T> supplier) {
- return cache.computeIfAbsent(groupId, artifactId, version, tag, supplier);
+ @FunctionalInterface
+ private interface ThrowingSupplier<T, E extends Exception> {
+ T get() throws E;
+ }
+
+ private static class ThrowingSupplierWrapper<T, E extends Exception> implements Supplier<T> {
+ private final ThrowingSupplier<T, E> throwingSupplier;
+
+ private ThrowingSupplierWrapper(ThrowingSupplier<T, E> throwingSupplier) {
+ this.throwingSupplier = throwingSupplier;
+ }
+
+ @Override
+ public T get() {
+ try {
+ return throwingSupplier.get();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static <T, E extends Exception> T process(
+ ThrowingSupplier<T, E> throwingSupplier, Function<Supplier<T>, T> consumer) throws E {
+ try {
+ return consumer.apply(new ThrowingSupplierWrapper<>(throwingSupplier));
+ } catch (RuntimeException e) {
+ if (e.getClass().equals(RuntimeException.class) && e.getCause() != null) {
+ throw (E) e.getCause();
+ } else {
+ throw e;
+ }
+ }
+ }
+ }
+
+ private <T, E extends Exception> T cache(
+ String groupId, String artifactId, String version, String tag, ThrowingSupplier<T, E> supplier)
+ throws E {
+ return ThrowingSupplierWrapper.process(
+ supplier, s -> cache.computeIfAbsent(groupId, artifactId, version, tag, s));
}
Review Comment:
This is exactly the kind of boiler plate code I wanted to avoid using runtime 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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Exception usage cleanup [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #1910:
URL: https://github.com/apache/maven/pull/1910#issuecomment-2479834654
Ok but @gnodet we need to pick up at least the bugfix from this PR, and consider maybe some others (you are catching ModelBuildingException while in fact ModelBuilderException is being thrown, etc).
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] There are exceptions you want to deal with [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1910:
URL: https://github.com/apache/maven/pull/1910#discussion_r1842178580
##########
impl/maven-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelBuilder.java:
##########
@@ -1733,12 +1734,50 @@ ModelSource resolveReactorModel(String groupId, String artifactId, String versio
return null;
}
- private <T> T cache(String groupId, String artifactId, String version, String tag, Supplier<T> supplier) {
- return cache.computeIfAbsent(groupId, artifactId, version, tag, supplier);
+ @FunctionalInterface
+ private interface ThrowingSupplier<T, E extends Exception> {
+ T get() throws E;
+ }
+
+ private static class ThrowingSupplierWrapper<T, E extends Exception> implements Supplier<T> {
+ private final ThrowingSupplier<T, E> throwingSupplier;
+
+ private ThrowingSupplierWrapper(ThrowingSupplier<T, E> throwingSupplier) {
+ this.throwingSupplier = throwingSupplier;
+ }
+
+ @Override
+ public T get() {
+ try {
+ return throwingSupplier.get();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static <T, E extends Exception> T process(
+ ThrowingSupplier<T, E> throwingSupplier, Function<Supplier<T>, T> consumer) throws E {
+ try {
+ return consumer.apply(new ThrowingSupplierWrapper<>(throwingSupplier));
+ } catch (RuntimeException e) {
+ if (e.getClass().equals(RuntimeException.class) && e.getCause() != null) {
+ throw (E) e.getCause();
+ } else {
+ throw e;
+ }
+ }
+ }
+ }
+
+ private <T, E extends Exception> T cache(
+ String groupId, String artifactId, String version, String tag, ThrowingSupplier<T, E> supplier)
+ throws E {
+ return ThrowingSupplierWrapper.process(
+ supplier, s -> cache.computeIfAbsent(groupId, artifactId, version, tag, s));
}
Review Comment:
IMO, the latest push is quite sensible. Also, have to add "boilerplate code wanted to avoid" vs all the changes revealed just by the compiler, as exception when from unchecked to checked... which one is worth more for you?
##########
impl/maven-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelBuilder.java:
##########
@@ -1733,12 +1734,50 @@ ModelSource resolveReactorModel(String groupId, String artifactId, String versio
return null;
}
- private <T> T cache(String groupId, String artifactId, String version, String tag, Supplier<T> supplier) {
- return cache.computeIfAbsent(groupId, artifactId, version, tag, supplier);
+ @FunctionalInterface
+ private interface ThrowingSupplier<T, E extends Exception> {
+ T get() throws E;
+ }
+
+ private static class ThrowingSupplierWrapper<T, E extends Exception> implements Supplier<T> {
+ private final ThrowingSupplier<T, E> throwingSupplier;
+
+ private ThrowingSupplierWrapper(ThrowingSupplier<T, E> throwingSupplier) {
+ this.throwingSupplier = throwingSupplier;
+ }
+
+ @Override
+ public T get() {
+ try {
+ return throwingSupplier.get();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static <T, E extends Exception> T process(
+ ThrowingSupplier<T, E> throwingSupplier, Function<Supplier<T>, T> consumer) throws E {
+ try {
+ return consumer.apply(new ThrowingSupplierWrapper<>(throwingSupplier));
+ } catch (RuntimeException e) {
+ if (e.getClass().equals(RuntimeException.class) && e.getCause() != null) {
+ throw (E) e.getCause();
+ } else {
+ throw e;
+ }
+ }
+ }
+ }
+
+ private <T, E extends Exception> T cache(
+ String groupId, String artifactId, String version, String tag, ThrowingSupplier<T, E> supplier)
+ throws E {
+ return ThrowingSupplierWrapper.process(
+ supplier, s -> cache.computeIfAbsent(groupId, artifactId, version, tag, s));
}
Review Comment:
IMO, the latest push is quite sensible. Also, have to add "boilerplate code wanted to avoid" vs all the changes revealed just by the compiler, as exception went from unchecked to checked... which one is worth more for you?
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Exception usage cleanup [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #1910:
URL: https://github.com/apache/maven/pull/1910
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] There are exceptions you want to deal with [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1910:
URL: https://github.com/apache/maven/pull/1910#issuecomment-2476773834
I really don't think that works. What if the ModelBuilder calls internally a service which throws an unchecked exception ? If the caller catches ModelBuilderException (which he would have to because that one is checked), then it may miss the unchecked exceptions. I don't really see a good justification for having only a few ones being unchecked or checked. That could work if there were real differences, such as transient issues (as in downloading and a network error happens), but most exceptions here are not of this kind.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org