Skip to content
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

Expose externalProjectPath on ExternalSystemNotificationExtension.customize() #2427

Conversation

vmadalin
Copy link
Contributor

@vmadalin vmadalin commented May 10, 2023

This was added to be able to support multiple gradle root projects, providing the externalProjectPath on the ExternalSystemNotification.customize allowing to provide better error message to users

@vmadalin vmadalin marked this pull request as ready for review May 10, 2023 14:05
@HackerMadCat
Copy link
Member

Hi! @vmadalin I cannot merge this pull request. There are a lot of compilation and usages conflicts.

  1. ExternalSystemNotificationExtension#customize without path is used inside Pants external plugin. So we need to deprecate old one method.
  2. It has conflicts with android part. Android also has a lot of usages of customize and createFailureResult. Inside: GradleBuildInvoker, GradleJvmNotificationExtension and GradleJvmNotificationExtensionTest

Please fix it and attach minimized Android commits.

@vmadalin
Copy link
Contributor Author

vmadalin commented Sep 5, 2023

Hi @HackerMadCat, thank you for reviewing the changes. I'm adding an update about them:

  1. ExternalSystemNotificationExtension#customize without path is used inside Pants external plugin. So we need to deprecate old one method.

Seems you are referring to https://github.com/pantsbuild/intellij-pants-plugin but definitely there are more plugins that may be affected by this. However, I marked the method as deprecated but also with empty implementation avoiding to require implementation by default

  1. It has conflicts with android part. Android also has a lot of usages of customize and createFailureResult. Inside: GradleBuildInvoker, GradleJvmNotificationExtension and GradleJvmNotificationExtensionTest

I created the following PR JetBrains/android#24 that resolving android conflicts on your android fork

@vmadalin vmadalin force-pushed the expose-external-project-path-to-system-notification branch from ebc1939 to 8deb2dd Compare October 3, 2023 12:03
@vmadalin vmadalin deleted the expose-external-project-path-to-system-notification branch October 5, 2023 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants