Updated KtLintConfigurator to check for new report file name for #201.

novoda / gradle-static-analysis-plugin

Easy setup of static analysis tools for Android and Java projects.

Apache License 2.0

410 stars 28 forks source link

Updated KtLintConfigurator to check for new report file name for #201. #202

Closed AdamMc331 closed 5 years ago

AdamMc331 commented 5 years ago

Currently this plugin won't support version 9.0.0+ from the ktlint-gradle plugin: #201

This PR resolves it by:

Adds version 9.0.0 to the integration tests, and updated them to make sure we were showing the right config for the version.
Added an if/else statement that handles the renamed property, and will now also throw a more relevant error if it were to change again.
Updated the gradle version to 5.x because that's required by the latest ktlint-gradle plugin.
Updated the sample app to point to version 9.0.0

Version 5.6.2 might be a larger jump than necessary, but I wasn't sure how to determine what a good change was. For ktlint-gradle the minimum supported version is 5.4.1, so let me know if we should use that instead.

Validated by updating to 9.0.0 in the sample app and ensuring that runs, as well as running all of the tests locally.

hal9002 commented 5 years ago

Can one of the admins review this PR?

tasomaniac commented 5 years ago

@hal9002 ok to test

tasomaniac commented 5 years ago

ok to test

tasomaniac commented 5 years ago

Here goes my failed attempts to trigger CI checks. @mr-archano can you help?

AdamMc331 commented 5 years ago

@tasomaniac I've made your suggested changes. I've also noticed that my git history looks a little weird, and I think that's because on my fork I rebased off of master and not develop.

Let me know if you want me to correct this & rebase off of develop, and I can do that once we've reviewed these changes.

tasomaniac commented 5 years ago

Also @AdamMc331, can you rebase into develop and only keep your commit(s)? The default branch is master because we want people to see the docs in master. I just changed the base branch to develop for this PR and that included some more commits (mostly docs and sample related.)

AdamMc331 commented 5 years ago

All set! I didn't squash mine but let me know if that's preferred and I can do this again. 🙈

tasomaniac commented 5 years ago

CI is running. Fingers crossed 🤞 As we tried #197 before, we had memory issues. Now i'm also trying here #203. With #203, I did some optimizations on tests to consume less memory. If that goes well, we can merge both and do a release.

Btw, with this change, our Gradle requirement does not change. Since we don't have hard-dependency to ktlint, we will still be supporting Gradle 4.10 and above.

AdamMc331 commented 5 years ago

@tasomaniac Are these the same memory related issues you've seen before?

tasomaniac commented 5 years ago

Yep. Finally had success in #203. Now I need a review/approval. Will ping @mr-archano for that and we can release this soon.

AdamMc331 commented 5 years ago

Great, once that is resolved just ping me and I will rebase this again.

tasomaniac commented 5 years ago

Thanks to @Mecharyry it is actually done. Merging once CI is good.

Mecharyry commented 5 years ago

Test this please oh mighty CI, destroyer of dreams

AdamMc331 commented 5 years ago

@tasomaniac Looks like we're all good! Thanks so much for your review and help on this. :)

tasomaniac commented 5 years ago

Thank you for the contribution. 🙌 Prepared PR for releasing version 1.1 #206