Generalize ForbiddenAPIs to the whole build #48655
Conversation
quarkus-bot
bot
added
area/arc
|
/cc @brunobat (opentelemetry), @radcortez (opentelemetry) |
|
|
||
| @SuppressForbidden(reason = "Type.toString() is what we need here") | ||
| public static Name from(Type type) { | ||
| return new Name(type.toString(), createSimpleName(type)); |
There was a problem hiding this comment.
Name seems to be used for things like beans, scopes, qualifiers etc. where type annotations are not common. (Note that Type.toString() only contains type annotations.) So this seems good enough to me.
(Using Type.name() here would drop more than just type variables; it would turn List<String> to List for example.)
There was a problem hiding this comment.
where type annotations are not common
I've never seen any type annotations in any CDI code. So I'd say it's pretty rare and not uncommon ;-).
So this seems good enough to me.
+1
| return (boolean) Console.class.getMethod("isTerminal").invoke(console); | ||
| } catch (Exception e) { | ||
| LOGGER.log(Level.SEVERE, "Failed to invoke System.console().isTerminal() via Reflection API", e); | ||
| LOGGER.error("Failed to invoke System.console().isTerminal() via Reflection API", e); |
There was a problem hiding this comment.
@phillip-kruger I wasn't sure if the use of java.util.logging here was something you actually wanted (maybe you need low level things?).
|
🎊 PR Preview 1a01fee has been successfully built and deployed to https://quarkus-pr-main-48655-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
Using the common signatures that were preexisting.
And drop our custom annotations.
At least most of the time.
gsmet
force-pushed
the
generalize-forbidden-apis
branch
from
45f3db7 to
190c4ac
Compare
|
Pushed something that should fix the first CI issue that happened very early. Let's see how it goes with the rest. |
Status for workflow
|
Status for workflow
|
|
@gastaldi I would appreciate if you could have a look as this will unblock a PR by @holly-cummins too. |
| LOG.debug("available exporters: " + exporters.stream() | ||
| .map(e -> e.getClass().getName()) | ||
| .reduce((a, b) -> a + ", " + b) | ||
| .orElse("none")); |
There was a problem hiding this comment.
Maybe avoid concatenating strings. I'm always confused with what parameter to use here ({}, {0}, %s)? I think it's %s given the other examples in the code
| LOG.debug("available exporters: " + exporters.stream() | |
| .map(e -> e.getClass().getName()) | |
| .reduce((a, b) -> a + ", " + b) | |
| .orElse("none")); | |
| LOG.debugf("available exporters: %s", exporters.stream() | |
| .map(e -> e.getClass().getName()) | |
| .reduce((a, b) -> a + ", " + b) | |
| .orElse("none")); |
There was a problem hiding this comment.
I think in this case, having the call protected with isDebugEnabled() is better.
There was a problem hiding this comment.
I don't see how that relates to my suggestion, as it's a simple enhancement to minimize String pooling/creation
There was a problem hiding this comment.
It's related because, before my change, this code was always executed, it's not now.
Feel free to adjust this further if you want, I didn't want a full CI run for something that is now good given it won't be executed if not in debug.
If you do it in a small PR, CI will be a lot more focused.
There was a problem hiding this comment.
Sure, the idea is not to avoid the code from being executed, more than to minimize the String concatenation cost. I'll submit a separate PR for that. Thanks
There was a problem hiding this comment.
Yeah, I'm just saying you only pay the cost in debug so you don't really care :).
But I'm fine with a follow-up PR, I agree the *f versions are cleaner.
4 participants
Using the common signatures that were preexisting for a start.
And add some more.