Revert "Revert "Defer projection failure until after plugins run (#1416)" (#1429)" #1430
Conversation
…thy-lang#1416)" (smithy-lang#1429)" This reverts commit 18676ec.
| projectedModel, resolvedModel, modelResult, resultBuilder); | ||
| } catch (Throwable e) { | ||
| LOGGER.severe(String.format("Plugin `%s` failed: %s", entry.getKey(), e)); |
There was a problem hiding this comment.
I think this would output the first error twice since we're still throwing it and logging it now. Would be good to see how this ends up looking in practice. If it's not good, then... don't log the first error?
There was a problem hiding this comment.
Yea, it logs every plugin failure, but only throws the first one. This comment on the original PR explains why I made that choice, but in short it is to show all plugin failures instead of just the first one. Here's a comparison of what it looks like:
Current state
INFO: Applying `broken` plugin to `source` projection
1 Smithy build projections failed.
(source): java.lang.RuntimeException: broken
software.amazon.smithy.build.SmithyBuildException: 1 Smithy build projections failed.
(source): java.lang.RuntimeException: broken
Logging plugin errors (what this PR does)
INFO: Applying `broken` plugin to `source` projection
SEVERE: Plugin `broken` failed: java.lang.RuntimeException: broken
INFO: Applying `build-info` plugin to `source` projection
INFO: Applying `model` plugin to `source` projection
INFO: Applying `sources` plugin to `source` projection
INFO: Writing empty `source` manifest because no Smithy sources found
INFO: Applying `test1Serial` plugin to `source` projection
1 Smithy build projections failed.
(source): java.lang.RuntimeException: broken
software.amazon.smithy.build.SmithyBuildException: 1 Smithy build projections failed.
(source): java.lang.RuntimeException: broken
Not logging the plugin errors
INFO: Applying `broken` plugin to `source` projection
INFO: Applying `build-info` plugin to `source` projection
INFO: Applying `model` plugin to `source` projection
INFO: Applying `sources` plugin to `source` projection
INFO: Writing empty `source` manifest because no Smithy sources found
INFO: Applying `test1Serial` plugin to `source` projection
1 Smithy build projections failed.
(source): java.lang.RuntimeException: broken
software.amazon.smithy.build.SmithyBuildException: 1 Smithy build projections failed.
(source): java.lang.RuntimeException: broken
Like I mentioned in the comment on the original commit, I think it would be better if we aggregated all plugin failures into one exception.
There was a problem hiding this comment.
I think @mtdowling is asking to not log this if the firstPluginError is null, since that one will end up being thrown.
There was a problem hiding this comment.
I've opened #1762 with those changes. Looking at the output now, I think its nicer to not log the first error since it doesn't do that right now.
|
Closing this in favor of #1762 to avoid merge conflicts. |
This reverts commit 18676ec.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.