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.

Sign up for GitHub

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

Partially refactor to use more idiomatic Gradle (part 2) #242

Merged

szpak merged 18 commits into gradle-nexus:master from TWiStErRob:idg

Jul 14, 2023

Collaborator

TWiStErRob commented Jul 8, 2023 •

edited

Loading

Following #241

Recommended to look at the changes commit by commit so the changes make more sense. In the general the goals were:

Remove constructor-injection, the Task should be reusable and self-contained, where the data is coming from (i.e. wiring) is the problem of the plugin, not the task.
Use the lazy and generated APIs as much as possible.
Minor cleanups while doing the above. (e.g. use Kotlin stdlib more, add groups/description for each task)

Breaking changes:

ReleaseNexusStagingRepository.setStagingRepositoryId renamed to setStagingRepositoryIdToRelease
CloseNexusStagingRepository.setStagingRepositoryId renamed to setStagingRepositoryIdToClose
All descendants of AbstractNexusStagingRepositoryTask have different constructors.

TWiStErRob added 15 commits

July 8, 2023 11:26


          Move comment to a more relevant place.

5b9f31d


          Replace some throw IllegalStateException with Kotlin's error()

ecd111e


          Hide utility function

b4032e0


          More explicit types

defa76e


          Be more specific about reflection.

c2e54cd

invalidateDescriptor has existed since Gradle 4.10 gradle/gradle@cea96d4

Side effect of this is that it'll throw NoSuchMethodException if the method is not found so the warning will show instead of just silently (?.) ignoring it.


          Simplify very long line

5fa5ea4


          Simplify Kotlin DSL usage

5e5cbca


          Clarify who's "this".

51e463c


          Move task setup closer to their creation and ensure all tasks have a …

f4fec09

…group and description.


          Remove all ObjectFactory usages from tasks.

5c034d5

Note: Gradle gets confused about a mutable property when generating the decorated code. Renaming the setters detaches it from the real property.

Example clash:
AbstractTransitionNexusStagingRepositoryTask.stagingRepositoryId -> getStagingRepositoryId: Property<String>
ReleaseNexusStagingRepository.setStagingRepositoryId -> setStagingRepositoryId(String)
together these two form a broken property.


          Remove last injected Extensions in tasks

6af4ca8


          Remove constructor-injected Repository

dd632c0


          Reduce constructor injection of registry by wiring the convention out…

548b707

…side of the task.


          Reduce project.provider { } and .get() calls, and use lazy API instead.

ef4ae20


          Extract task creation for each repository to make inputs more explicit.

777dbde

TWiStErRob commented

View reviewed changes

Collaborator Author

TWiStErRob left a comment

Few notes for reviewers.

src/main/kotlin/io/github/gradlenexus/publishplugin/ReleaseNexusStagingRepository.kt Show resolved Hide resolved

src/main/kotlin/io/github/gradlenexus/publishplugin/CloseNexusStagingRepository.kt Show resolved Hide resolved

src/main/kotlin/io/github/gradlenexus/publishplugin/ReleaseNexusStagingRepository.kt Show resolved Hide resolved

src/main/kotlin/io/github/gradlenexus/publishplugin/NexusPublishPlugin.kt Show resolved Hide resolved

src/main/kotlin/io/github/gradlenexus/publishplugin/NexusPublishPlugin.kt Outdated

Comment on lines 132 to 136

                           val findStagingRepository = rootProject.tasks.register<FindStagingRepository>(
                               "find${capitalizedName}StagingRepository",
-                              rootProject.objects,
-                              extension,
-                              repository,
                               registry
                           )
                           findStagingRepository {

Collaborator Author

TWiStErRob Jul 8, 2023 •

edited

Loading

My goal was to merge each of these into:

val name = rootProject.tasks.register<T>("name") {
    ...
}

but these last two I can't figure out how to remove the registry.

I think the answer is using the build service (injecting it inside the task), but that's only possible from Gradle 6.1 (see NexusPublishPlugin.createRegistry) and the current minimum is 6.0.

@3flex any ideas?

Collaborator Author

TWiStErRob Jul 13, 2023 •

edited

Loading

Just moved from unannotated constructor property to @Internal managed property: e964697

src/main/kotlin/io/github/gradlenexus/publishplugin/NexusPublishPlugin.kt Outdated Show resolved Hide resolved

src/main/kotlin/io/github/gradlenexus/publishplugin/NexusPublishPlugin.kt Outdated Show resolved Hide resolved

TWiStErRob force-pushed the idg branch from bc35e55 to 777dbde Compare

July 8, 2023 19:22

TWiStErRob commented

View reviewed changes

src/main/kotlin/io/github/gradlenexus/publishplugin/CloseNexusStagingRepository.kt Show resolved Hide resolved

szpak requested changes

View reviewed changes

Contributor

szpak left a comment

Thanks @TWiStErRob. It looks good, I've just made a few nitpicking.

Btw, as my other plugins are still at the Gradle 6 level of "idiomaticity", @3flex maybe you could also take a look?

src/main/kotlin/io/github/gradlenexus/publishplugin/CloseNexusStagingRepository.kt Show resolved Hide resolved

src/main/kotlin/io/github/gradlenexus/publishplugin/NexusPublishPlugin.kt Outdated Show resolved Hide resolved

src/main/kotlin/io/github/gradlenexus/publishplugin/NexusPublishExtension.kt Show resolved Hide resolved

...github/gradlenexus/publishplugin/internal/InvalidatingStagingRepositoryDescriptorRegistry.kt Show resolved Hide resolved

src/main/kotlin/io/github/gradlenexus/publishplugin/NexusPublishPlugin.kt Show resolved Hide resolved

Collaborator Author

TWiStErRob commented Jul 11, 2023

Btw, as my other plugins are still at the Gradle 6 level of "idiomaticity"

Btw, this is motivated by gradle/gradle#22348 and https://docs.google.com/document/d/1iZHN5mcN56IQtGOoR5GyH9NQLSOQLQ1SHN5ulNheDjc which is going towards "managed everything".

Collaborator Author

TWiStErRob commented Jul 11, 2023

@szpak Ok, so in the end there were no changes necessary, the only finding you flagged was in uncommitted code 😅.

TWiStErRob requested a review from szpak

July 11, 2023 15:48

Collaborator Author

TWiStErRob commented Jul 11, 2023 •

edited

Loading

I consider this ready, pending a word from @3flex. Btw, that word could be "go ahead, I'm not going to review" too if you don't have time.

TWiStErRob mentioned this pull request

Rename Tasks to have Task suffix TWiStErRob/nexus-publish-gradle-plugin#6

Closed

TWiStErRob added 3 commits

July 13, 2023 09:44


          Move to managed property

e964697


          Unhide only hidden property in the project

c4ad618


          Merge remote-tracking branch 'origin/master' into idg

TWiStErRob mentioned this pull request

Gradle 6.2 as minimum TWiStErRob/nexus-publish-gradle-plugin#7

Closed

szpak approved these changes

View reviewed changes

szpak mentioned this pull request

Random fixes #245

Merged

szpak merged commit 66f3284 into gradle-nexus:master

6 checks passed

Collaborator Author

TWiStErRob commented Jul 14, 2023

Ugh, should I rebase and squash a few next time to not get so many random small commits to master?

Contributor

szpak commented Jul 14, 2023

Ugh, should I rebase and squash a few next time to not get so many random small commits to master?

No, I did it purposely. I wanted to keep some of those smaller changes (to maybe replicate in my other plugins ;) ). Sure, some commits could be squashed/rebased, but I didn't want to bother you.
In general, a "squash and commit" is a default strategy set for the project.

This was referenced Jul 18, 2023

Breaking change: rename Tasks to have Task suffix #250

Closed

Gradle 6.2 as minimum #251

Merged

TWiStErRob deleted the idg branch

July 18, 2023 16:24

TWiStErRob mentioned this pull request

Gradle 6.2 as minimum (re-merge of #251) #274

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet