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

Stop configuring report merge tasks while configuring Detekt tasks #5813

Merged
merged 2 commits into from
Feb 23, 2023

Conversation

3flex
Copy link
Member

@3flex 3flex commented Feb 22, 2023

See point 2: https://docs.gradle.org/8.0.1/userguide/task_configuration_avoidance.html#sec:task_configuration_avoidance_general

Only mutate the current task inside a configuration action... Mutating anything other than the current task can cause indeterminate behavior in your build.

Also see: #5772 (comment).

Docs should be updated to align - that can be done in a new commit once these changes are reviewed & approved.

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Merging #5813 (ece4374) into main (2290ec8) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #5813   +/-   ##
=========================================
  Coverage     84.61%   84.61%           
  Complexity     3787     3787           
=========================================
  Files           546      546           
  Lines         12918    12918           
  Branches       2268     2268           
=========================================
  Hits          10930    10930           
  Misses          862      862           
  Partials       1126     1126           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

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

LGTM, tested as well, behaviors were as expected. Please update docs, and reply to conversations.

See point 2: https://docs.gradle.org/8.0.1/userguide/task_configuration_avoidance.html#sec:task_configuration_avoidance_general

> Only mutate the current task inside a configuration action... Mutating
> anything other than the current task can cause indeterminate behavior in
> your build.
Copy link
Member

@chao2zhang chao2zhang left a comment

Choose a reason for hiding this comment

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

TIL

Did you catch this bug manually or there are new warnings from Gradle 8?

@github-actions
Copy link

Warnings
⚠️ This PR is approved with no milestone set. If merged, it won't appear in the Detekt release notes.
Messages
📖

Thank you very much for making our website better ❤️!

Generated by 🚫 dangerJS against ece4374

@3flex
Copy link
Member Author

3flex commented Feb 23, 2023

Did you catch this bug manually or there are new warnings from Gradle 8?

It was caught after #5772 was merged - I don't think it's related to Gradle 8. That warning from the Gradle docs that I referenced has been there for some time, I just haven't had a closer look until I made the change in #5772, not realising that the tasks had to be eagerly configured with the current way of configuring report merging.

@3flex 3flex added this to the 1.23.0 milestone Feb 23, 2023
@TWiStErRob TWiStErRob merged commit 79eaed6 into detekt:main Feb 23, 2023
@3flex 3flex deleted the simplify-merging branch February 23, 2023 10:10
TWiStErRob added a commit to TWiStErRob/net.twisterrob.colorfilters that referenced this pull request Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants