Fix Internal API usage of PlatformUtils #179
Conversation
Replaced internal PlatformUtils usage with System.getProperty("idea.platform.prefix") check.
This resolves 3 internal API usage warnings in the plugin verification report.
| public static boolean isIdeWithMultipleModuleSupport() { | ||
| return PlatformUtils.isIntelliJ() || "AndroidStudio".equals(PlatformUtils.getPlatformPrefix()); | ||
| String prefix = System.getProperty("idea.platform.prefix"); | ||
| return prefix == null || prefix.startsWith("Idea") || "AndroidStudio".equals(prefix); |
There was a problem hiding this comment.
It sort of seems like we should return false if prefix is null. Or? (EDIT: see below.)
Also: are we confident enough in our testing that we'd catch it if this implementation detail changed or do we think an "idea.platform.prefix" system property is a durable promise?
In general, this is an interesting kind of fix and I wonder if we shouldn't add a comment noting that we've inlined internal implementation (over using an internal API). I guess it's situational but I would generally hope for something more future-safe. Could we ask the prompt to try harder?
Regarding my first comment, looking at the implementation of getPlatformPrefix, it does default to "idea" if the property is unset so I guess this is probably behavior-preserving but that seems kind of odd. Maybe even a bug in their implementation? If not a bug today, are we confident it will stay true in the future?
There was a problem hiding this comment.
TL;DR: in general I'd minimally want a comment explaining where the magic is coming from; maximally, prefer a proper API and ask Gemini to try harder to find one
There was a problem hiding this comment.
Looking closer at the API docs, it looks like there actually is a preferred way to handle this...
See:
A little onerous, but prescribed. Worth evaluating at least!
There was a problem hiding this comment.
I've thought a lot about this and finally came around to this implementation. If we were to get our verifications down to zero in all categories then with future changes we'd be safe-guarded against new entries in a way that we aren't today.
As for this change, I believe that we actually want to not need these kinds of checks at all, this change preserved the old functionality in a way that is more future proof which I thought was important.
There was a problem hiding this comment.
If we were to get our verifications down to zero in all categories then with future changes we'd be safe-guarded against new entries in a way that we aren't today.
You mean because if we had zero violations we could have new failures break the build? I totally agree that we want to get there but I don't think we want to take on technical debt along the way. Or maybe I'm misunderstanding your point?
As for this change, I believe that we actually want to not need these kinds of checks at all, this change preserved the old functionality in a way that is more future proof which I thought was important.
How is it future-proof? The change proposes inlining an implementation that could change at any time. Or maybe I'm misunderstanding? Anyway, this may all be besides the point since the docs suggest there's actually a way to do this that only uses public API which should be really future-proof. Or maybe you evaluated and disqualified that approach?
2 participants
Replaced internal PlatformUtils usage with System.getProperty("idea.platform.prefix") check. This resolves 3 internal API usage warnings in the plugin verification report.