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

Jump to bottom

Add Upgrade Note for Strongly Typed Dependencies in Test Suites #22778

Merged
merged 9 commits into from Nov 17, 2022
Merged

Add Upgrade Note for Strongly Typed Dependencies in Test Suites #22778

merged 9 commits into from Nov 17, 2022

Conversation

tresat
Copy link
Member

@tresat tresat commented Nov 17, 2022

Fixes #22720

@tresat tresat self-assigned this Nov 17, 2022
@tresat tresat added the in:documentation DO NOT USE label Nov 17, 2022
@tresat tresat added this to the 7.6 RC4 milestone Nov 17, 2022
@tresat tresat requested review from a team and ghale November 17, 2022 14:33
@tresat
Copy link
Member Author

tresat commented Nov 17, 2022

@bot-gradle test bd

@bot-gradle
Copy link
Collaborator

OK, I've already triggered the following builds for you:

@tresat tresat requested a review from ljacomet November 17, 2022 14:34
@tresat tresat marked this pull request as ready for review November 17, 2022 14:34
ljacomet
ljacomet requested changes Nov 17, 2022
View reviewed changes
Copy link
Member

@ljacomet ljacomet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, but one suggestion and one issue, see below

subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
@tresat tresat requested a review from ljacomet November 17, 2022 15:43
nathan-contino
nathan-contino approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@nathan-contino nathan-contino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs LGTM -- optional suggestions.

subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
@tresat
Copy link
Member Author

tresat commented Nov 17, 2022

I tried to combine and apply Nate and Sterling's suggestions, thank you both.

nathan-contino
nathan-contino approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@nathan-contino nathan-contino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs LGTM, but I hope you accept my suggestions to streamline this section a bit.

subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated
@@ -117,6 +117,59 @@ PMD has been updated to https://pmd.github.io/pmd-6.48.0/pmd_release_notes.html[
When configuring an executable explicitly for link:{groovyDslPath}/org.gradle.api.tasks.compile.ForkOptions.html#org.gradle.api.tasks.compile.ForkOptions:executable[`JavaCompile`] or link:{groovyDslPath}/org.gradle.api.tasks.testing.Test.html#org.gradle.api.tasks.testing.Test:executable[`Test`] tasks, Gradle will now emit an error if this executable does not exist.
In the past, the task would be executed with the default toolchain or JVM running the build.

==== Changes to dependency declarateions in Test Suites

As part of the ongoing effort to improve Test Suites, dependency declarations using its nested `dependencies` block are <<jvm_test_suite_plugin.adoc#sec:differences_with_top_level_dependencies, now strongly typed>>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
As part of the ongoing effort to improve Test Suites, dependency declarations using its nested `dependencies` block are <<jvm_test_suite_plugin.adoc#sec:differences_with_top_level_dependencies, now strongly typed>>.
Dependency declarations in the Test Suites `dependencies` block are <<jvm_test_suite_plugin.adoc#sec:differences_with_top_level_dependencies, now strongly typed>>.

Suggestion: as an incubating API, I assume users are aware we are continually tweaking and improving things. Better to avoid the confusing pronoun usage, eliminate the intro, and keep the sentence trim.

Copy link
Member Author

@tresat tresat Nov 17, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the intro here. I think part of the reason for this note existing is to head of criticism in the form of "Why did this minor version upgrade suddenly break my build?" Re-emphasizing the incubating and changing status of the API seems worthwhile, and might prevent some angry feedback/issues. I like the changes after that clause.

subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
tresat and others added 4 commits November 17, 2022 13:57
@tresat
Update subprojects/docs/src/docs/userguide/migration/upgrading_versio…
0df577d 
…n_7.adoc

Co-authored-by: nate contino <ncontino@gradle.com>
@tresat
Update subprojects/docs/src/docs/userguide/migration/upgrading_versio…
716906d 
…n_7.adoc

Co-authored-by: nate contino <ncontino@gradle.com>
@tresat
Update subprojects/docs/src/docs/userguide/migration/upgrading_versio…
10302c4 
…n_7.adoc

Co-authored-by: nate contino <ncontino@gradle.com>
@tresat
Correct typo, phrasing
d14cc44
@big-guy
Copy link
Member

big-guy commented Nov 17, 2022

@bot-gradle test and merge

@gradle gradle deleted a comment from tresat Nov 17, 2022
@bot-gradle
Copy link
Collaborator

OK, I've already triggered a build for you.

@bot-gradle bot-gradle merged commit 1959236 into release Nov 17, 2022
@blindpirate blindpirate deleted the tt/76/upgrade-note-for-project-deps-in-suites branch November 18, 2022 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljacomet ljacomet ljacomet approved these changes

@big-guy big-guy big-guy approved these changes

@nathan-contino nathan-contino nathan-contino approved these changes

@ghale ghale Awaiting requested review from ghale ghale was automatically assigned from gradle/bt-jvm

Assignees

@tresat tresat

Labels
in:documentation DO NOT USE
Projects
None yet
Milestone
7.6 RC4
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tresat @bot-gradle @big-guy @ljacomet @nathan-contino