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

Upgrade gradle to 7.0. #420

Merged
merged 2 commits into from
May 19, 2021
Merged

Upgrade gradle to 7.0. #420

merged 2 commits into from
May 19, 2021

Conversation

zach-klippenstein
Copy link
Collaborator

No description provided.

@zach-klippenstein zach-klippenstein requested a review from a team as a code owner May 10, 2021 15:12
@zach-klippenstein
Copy link
Collaborator Author

Seems like we finally need to get off our super-old Dokka version to unblock this.

@zach-klippenstein
Copy link
Collaborator Author

Dokka upgrade in #421 should unblock this.

@zach-klippenstein
Copy link
Collaborator Author

Looks like we need to upgrade the ktlint plugin as well, latest version is 10.0.0.

@zach-klippenstein
Copy link
Collaborator Author

zach-klippenstein commented May 11, 2021

And detekt, 1.16.0. Which now fails a bunch of stuff that was perfectly fine before…

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/upgrade-gradle branch from 7bb0fb2 to 1253b62 Compare May 11, 2021 01:22
@zach-klippenstein
Copy link
Collaborator Author

Wasn't able to figure out how to get detekt to actually generate a baseline, so shelving this for now.

@zach-klippenstein
Copy link
Collaborator Author

zach-klippenstein commented May 11, 2021

image
👀

I'm not sure we've ever really gotten value from detekt anyway. Ktlint is probably all we really need. @rjrjr how serious were you?

@rjrjr
Copy link
Contributor

rjrjr commented May 11, 2021 via email

zach-klippenstein added a commit that referenced this pull request May 18, 2021
This change was discussed and agreed upon in #420, since we don't get any value out of detekt and it makes maintaining our build scripts more difficult.
@zach-klippenstein
Copy link
Collaborator Author

Removed detekt in #426, rebasing this on top of that…

zach-klippenstein added a commit that referenced this pull request May 18, 2021
This change was discussed and agreed upon in #420, since we don't get any value out of detekt and it makes maintaining our build scripts more difficult.
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/upgrade-gradle branch from 1253b62 to 8daa358 Compare May 18, 2021 23:21
@zach-klippenstein zach-klippenstein requested a review from a team as a code owner May 18, 2021 23:21
@zach-klippenstein zach-klippenstein changed the base branch from main to zachklipp/remove-detekt May 18, 2021 23:22
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/upgrade-gradle branch from 8daa358 to 3f38b9b Compare May 18, 2021 23:56
@zach-klippenstein
Copy link
Collaborator Author

Updated jmh (benchmarking) and ktlint gradle plugins to their latest versions, required for Gradle 7.

@zach-klippenstein
Copy link
Collaborator Author

Added a separate commit that updates gradle and AGP for the compose project. I did both there because AGP was so far behind.

Comment on lines +97 to +110

// We had to disable the indent and parameter-list-wrapping rules, because they lead to
// false positives even in the most recent KtLint version. We created tickets:
//
// https://github.com/pinterest/ktlint/issues/963
// https://github.com/pinterest/ktlint/issues/964
// https://github.com/pinterest/ktlint/issues/965
//
// We can't revert the KtLint version, because they only work with Kotlin 1.3 and would
// block Kotlin 1.4. We rather have a newer Kotlin version than a proper indent. The
// indent rule needs to be disabled globally due to another bug:
// https://github.com/pinterest/ktlint/issues/967
"indent",
"parameter-list-wrapping"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just copied this in from the main project's build.gradle.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems we should be able to re-enable indent and parameter-list-wrapping (assuming we're on a recent-enough version of ktlint and there aren't any new Kotlin-compatibility issues) 🤞.

963 was allegedly fixed here; 964 here (transitively via 918); 965 here.

@zach-klippenstein zach-klippenstein requested review from rjrjr and a team May 19, 2021 00:30
Base automatically changed from zachklipp/remove-detekt to main May 19, 2021 00:58
Copy link
Collaborator

@maybeYouShouldBeTheLawyer maybeYouShouldBeTheLawyer left a comment

Choose a reason for hiding this comment

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

A couple comments about the disabled ktlint rules. Otherwise lgtm.

Comment on lines 95 to 96
// This is a known issue: https://github.com/pinterest/ktlint/issues/527
"import-ordering",
Copy link
Collaborator

Choose a reason for hiding this comment

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

A heads-up just in case you're interested in a follow-on PR:

It looks like the import-ordering issue has been (mostly) fixed thanks to the introduction of a layout table for Kotlin imports. (But note the misbehavior cited at the end of that conversation, including a comment by the tried-and-true Valeriy Ovechkin 😺 .)

Copy link
Collaborator Author

@zach-klippenstein zach-klippenstein May 19, 2021

Choose a reason for hiding this comment

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

I'm not interesting in diverging these effectively duplicate configs any more than necessary. We can follow up and address in the main project once these have merged.

Comment on lines +97 to +110

// We had to disable the indent and parameter-list-wrapping rules, because they lead to
// false positives even in the most recent KtLint version. We created tickets:
//
// https://github.com/pinterest/ktlint/issues/963
// https://github.com/pinterest/ktlint/issues/964
// https://github.com/pinterest/ktlint/issues/965
//
// We can't revert the KtLint version, because they only work with Kotlin 1.3 and would
// block Kotlin 1.4. We rather have a newer Kotlin version than a proper indent. The
// indent rule needs to be disabled globally due to another bug:
// https://github.com/pinterest/ktlint/issues/967
"indent",
"parameter-list-wrapping"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems we should be able to re-enable indent and parameter-list-wrapping (assuming we're on a recent-enough version of ktlint and there aren't any new Kotlin-compatibility issues) 🤞.

963 was allegedly fixed here; 964 here (transitively via 918); 965 here.

@zach-klippenstein zach-klippenstein merged commit 9088517 into main May 19, 2021
@zach-klippenstein zach-klippenstein deleted the zachklipp/upgrade-gradle branch May 19, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants