-
Notifications
You must be signed in to change notification settings - Fork 413
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
Add K2 analysis switcher #3152
Add K2 analysis switcher #3152
Conversation
f26b021
to
da4767c
Compare
da4767c
to
786de04
Compare
…grationTest, JsIRGradleIntegrationTest, Multiplatform0GradleIntegrationTest integrations tests due to KT-63126
d509eb1
to
f31d535
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have much to add to the switch itself, well done here!
I do have some comments for the test-related changes though. Given that Andrei is waiting for the switch to begin testing, could you please extract all test-related changes into a separate PR (or extract the switch into a separate one)? Then we can quickly merge the switch and unblock Andrei, and then have more time to discuss the tests.
It was also outlined in #3113 for this particular reason, so that we can implement it quickly and unblock testing
Setting up unit and integration tests to use the switch falls outside of the scope of this issue.
compileOnly(projects.subprojects.analysisKotlinApi) | ||
implementation(projects.subprojects.analysisKotlinApi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(rhetorical question) I wonder how and why it worked before...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was not merged in scope of this PR - is it correct? Without it, most likely switch to K2 will not work, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one way to find out :) #3333
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was not merged in scope of this PR - is it correct? Without it, most likely switch to K2 will not work, no?
Sorry, I have missed it( Thank you for noticing it. It would be nice to have integration tests in single PR.
(rhetorical question) I wonder how and why it worked before...
Do you mean unit tests? This dependency comes from pluginBaseTestUtils
where it is not excluded.
runners/gradle-plugin/src/main/kotlin/org/jetbrains/dokka/gradle/DokkaPlugin.kt
Outdated
Show resolved
Hide resolved
runners/gradle-plugin/src/main/kotlin/org/jetbrains/dokka/gradle/gradleConfigurations.kt
Outdated
Show resolved
Hide resolved
2cf4244
to
52b15ae
Compare
It is a part of #3113