Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

Detekt RC9 integration #134

Merged
merged 3 commits into from Oct 6, 2018
Merged

Detekt RC9 integration #134

merged 3 commits into from Oct 6, 2018

Conversation

tasomaniac
Copy link
Contributor

@tasomaniac tasomaniac commented Oct 2, 2018

Detekt Gradle Plugin was rewritten recently. It now extends from CodeQualityTask. This PR fixes the integration with the new version. From now on, I am not expecting the integration to be broken anymore.

Changes in the new plugin:

  • profile is gone. Since we gather user's configurations, this is not directly affecting us. But affects the tests drastically. That's why a copied version of the tests are added with the configuration. I could've been more intelligent on this but 1 test is not applicable so I decided to have a new copy.
  • detektCheck task is renamed to detekt
  • new reports property is available mimicking other Java tools. I was hoping they would extend from Reporting from gradle. It is a public API. This would make our integration even more solid. :) But you cannot have all the nice things. Currently depending on having this property availability.

Fixes #131

@rock3r
Copy link
Contributor

rock3r commented Oct 2, 2018

Nice one @tasomaniac, happy to approve once you sort out the failing tests :)

@tasomaniac
Copy link
Contributor Author

Oh I thought I commented before. So I don't have access to the CI anymore. Can you post the test output? Thanks.

@rock3r
Copy link
Contributor

rock3r commented Oct 2, 2018

Hm the CI should be public for this, let me check.

@rock3r
Copy link
Contributor

rock3r commented Oct 2, 2018

Fixed :)

@tasomaniac
Copy link
Contributor Author

Retest please

@tasomaniac
Copy link
Contributor Author

It's a DaemonDisappearedException. How do we restart the tests?

@mr-archano
Copy link
Contributor

@hal90002 retest this please

Copy link
Contributor

@mr-archano mr-archano left a comment

Choose a reason for hiding this comment

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

Pretty simple change, and full retro-compatible! I like it 👍

@mr-archano
Copy link
Contributor

@tasomaniac not sure what's going on with the CI, I will look at this again tomorrow once back at the office.

@mr-archano
Copy link
Contributor

@hal90002 retest this please

@mr-archano
Copy link
Contributor

@tasomaniac the issue with the build was the emoji (😄) in the PR description, I know that it sounds silly but this has stung me recently too. I removed that and now the new tests have failed because:

* What went wrong:
org.gradle.api.tasks.TaskContainer.register(Ljava/lang/String;Ljava/lang/Class;Lorg/gradle/api/Action;)Lorg/gradle/api/tasks/TaskProvider;

The new Detekt plugin is using the new TaskContainer.register(), but that breaks compatibility with Gradle < 4.9 (iirc). I have raised the question in the Detekt slack a while ago, need to follow-up to understand if we can ship a workaround for this.

@tasomaniac
Copy link
Contributor Author

Hmm, I don't think we would need to increase it in the plugin. I think the problem is the integration tests. In fact I run the tests locally with my global Gradle instance not with the wrapper.

Although at the same time, I don't think increasing the Gradle wrapper would hurt.

@mr-archano
Copy link
Contributor

@tasomaniac you're definitely right. Let me open a PR to bump the Gradle version to the latest. You can back merge once that is done.

@mr-archano
Copy link
Contributor

@tasomaniac I just opened the PR to update the Gradle wrapper(s) in the repo: #136. Once that is merged we can update and merge this PR too.

@mr-archano mr-archano merged commit 7f558c9 into develop Oct 6, 2018
@mr-archano mr-archano deleted the taso/fix-detekt9 branch October 6, 2018 13:18
@tasomaniac
Copy link
Contributor Author

Thanks 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants