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

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

Merged

Conversation

AdamMc331
Copy link
Contributor

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
Copy link
Contributor

hal9002 commented Oct 20, 2019

Can one of the admins review this PR?

@tasomaniac tasomaniac changed the base branch from master to develop October 21, 2019 18:49
tasomaniac
tasomaniac previously approved these changes Oct 21, 2019
Copy link
Contributor

@tasomaniac tasomaniac left a comment

Choose a reason for hiding this comment

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

Awesome. 🎉 Thanks for fixing this. I only have small suggestions. Let me know what you think and I will merge.

@tasomaniac
Copy link
Contributor

@hal9002 ok to test

@tasomaniac
Copy link
Contributor

ok to test

@tasomaniac
Copy link
Contributor

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

@AdamMc331
Copy link
Contributor Author

@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
Copy link
Contributor

tasomaniac commented Oct 21, 2019

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.)

tasomaniac
tasomaniac previously approved these changes Oct 21, 2019
@AdamMc331 AdamMc331 force-pushed the GSAP-201/support_allReportsOutputFiles branch from d8399ba to c41b42b Compare October 21, 2019 19:51
@AdamMc331
Copy link
Contributor Author

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

tasomaniac
tasomaniac previously approved these changes Oct 21, 2019
@tasomaniac
Copy link
Contributor

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
Copy link
Contributor Author

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

@tasomaniac
Copy link
Contributor

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
Copy link
Contributor Author

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

@tasomaniac
Copy link
Contributor

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

@Mecharyry
Copy link

Test this please oh mighty CI, destroyer of dreams

@AdamMc331
Copy link
Contributor Author

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

@tasomaniac tasomaniac merged commit db201b7 into novoda:develop Oct 21, 2019
@AdamMc331 AdamMc331 deleted the GSAP-201/support_allReportsOutputFiles branch October 21, 2019 21:11
@tasomaniac
Copy link
Contributor

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

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.

4 participants