-
Notifications
You must be signed in to change notification settings - Fork 485
Compatibility considerations for the IDE hook. #1103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ragurney
commented
Jan 31, 2022
Hi @nedtwigg. I don't have enough knowledge of the IntelliJ SDK know off-hand which would be easier -- I'll have to do some investigation -- but overall I think I prefer option A. It seems like it would provide a better user experience.
@ragurney added Gradle version parsing in ragurney/spotless-intellij-gradle#22.
One option to allow the speedup mentioned in 2. above (switching from spotlessApply
to :spotlessApply
would be for the Spotless plugin to alter behavior based on Gradle version, since both Spotless and its users can easily determine the Gradle version, whereas its harder for users to determine the version of Spotless.
badsyntax
commented
Feb 12, 2022
@nedtwigg apologies for the belated response. I'm going with option A. I've started the process to make this happen in vscode (see microsoft/vscode-gradle#1156) but haven't had time to chase it up. It's in the works though.
8b73df8
to
a0a35b2
Compare
The Spotless IDE hook should just be
spotlessApply -PspotlessIdeHook=${ABSOLUTE_PATH_TO_FILE}
. We've had this API since3.30.0
, and you shouldn't have to parse a Gradle or Spotless version to use it.But we've got two backward incompatible problems:
Spotless IDE hook is not compatible with the configuration-cache, so you have to specify
--no-configuration-cache
. But, that flag is not compatible with Gradle < 6.6. So you're stuck in this position where you have to parse the Gradle version to know whether you need that flag or not.We can speed Spotless up a lot by changing the API to
:spotlessApply -PspotlessIdeHook=BLAH
, but only after faster -PspotlessIdeHook with configure-on-demand #1101 gets implemented. So you're stuck in this position where you have to parse the Spotless version to know whether you can add the leading colon or not.We could fix both of those issues by changing the API to this:
:spotlessIdeHook -PspotlessIdeHook=BLAH --configure-on-demand
This fixes problem 1 because Gradle 7.4 adds a new
Task.notCompatibleWithConfigurationCache()
method. We can't use that method to fix problem 1 forspotlessApply
, but we could use it to fix a new task calledspotlessIdeHook
.It also fixes problem 2 because we can change the implementation of
:spotlessIdeHook
over time. At first it would trigger the evaluation of all subprojects (likespotlessApply
does now), but later on it could take advantage of configure-on-demand to speed up execution dramatically.So I have a question for each of you, @badsyntax and @ragurney, which is easier?
A) Parse Gradle and Spotless versions to determine which arguments to use.
B) Optimistically try
:spotlessIdeHook
method, if it fails fall back tospotlessApply
, and then remember "optimistic attempt failed" for the rest of the plugin's runtime. No parsing needed, just detecting an error.As an aside, Spotless is adding support for linters in the medium-term future, and those lints will be available in the IDE hook. We are able to add this information in a backward-compatible way, so it will be optional whether you choose to add support for that in future, no version-parsing or feature-detection required.