Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Compatibility considerations for the IDE hook. #1103

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nedtwigg
Copy link
Member

The Spotless IDE hook should just be spotlessApply -PspotlessIdeHook=${ABSOLUTE_PATH_TO_FILE}. We've had this API since 3.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:

  1. 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.

  2. 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 for spotlessApply, but we could use it to fix a new task called spotlessIdeHook.

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 (like spotlessApply 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 to spotlessApply, 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.

@ragurney
Copy link

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.

@nedtwigg
Copy link
Member Author

nedtwigg commented Feb 12, 2022

@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
Copy link

@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.

@nedtwigg nedtwigg changed the base branch from main-temp to main April 20, 2022 21:30
@nedtwigg nedtwigg marked this pull request as draft January 3, 2023 02:40
@nedtwigg nedtwigg added the pr-archive PRs which are still valid but have gotten stuck for some reason label Feb 12, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pr-archive PRs which are still valid but have gotten stuck for some reason
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants