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

build(deps): bump org.jlleitschuh.gradle.ktlint from 11.6.1 to 12.1.1 & ktlint 1.5.0 #17581

Merged
merged 7 commits into from
Dec 9, 2024

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Dec 9, 2024

Reviewers: please pay attention to the manual changes in the commits. They have been fully split out

Important

Please don't merge anything else until after this is merged, or will be a define "can't merge".
This is almost certainly going to conflict, and took a LOT of work to get into a good state

Note: this could be split into 2 PRs: one per dependency, but it felt sensible to combine the two, as was done in the linked commit

Fixes

Approach

  • Upgrade to org.jlleitschuh.gradle.ktlint 12.1.1
  • Upgrade to ktlint 1.5.0

Similar approach to both:

  1. Pre-fix
    • Perform the version upgrade, then ./gradlew --rerun-tasks ktLintFormat
    • Revert the version upgrade and run ./gradlew --rerun-tasks ktLintFormat again
    • One initial commit for manual fixups
    • A second commit for automated fixups
  2. Version bump + limited diff of automated changes via ./gradlew --rerun-tasks ktLintFormat

How Has This Been Tested?

CI.

Ran ./gradlew --rerun-tasks ktLintFormat on each commit. Success, and no changes

Learning (optional, can help others)

We really shouldn't let lint get out of date

⚠️ A potential compiler bug in CardBrowser caused by annotations on a when.
ktlint didn't seem to like it. Fixed in 1.5.0 I believe, but one to note

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

david-allison and others added 3 commits December 9, 2024 18:40
Appears to be caused by an annotation on a case
This prepares for the gradle plugin upgrade
And some for ktlint 1.5.0

after updating gradle.ktlint, the following command now succeeds
`./gradlew --rerun-tasks AnkiDroid:ktLintFormat`

Process:
* run `./gradlew AnkiDroid:ktlintFormat`
* find the file and line number
* rollback the changes (line numbers no longer match)
* fix the error
* amend to this commit disabling git hooks
* ignored a class of error regarding comment placement
  * common to have block comments after KDocs on classes

Some issues:

* Exceeded max line length (140) (cannot be auto-corrected)
* a block comment may not be preceded by a block comment (cannot be auto-corrected)
* a block comment may not be preceded by an EOL comment unless separated by a blank line (cannot be auto-corrected)
* an EOL comment may not be preceded by a block comment unless separated by a blank line (cannot be auto-corrected)
* an EOL comment may not be preceded by a KDoc. Reversed order is allowed though when separated by a newline. (cannot be auto-corrected)
* A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above. (cannot be auto-corrected)
* A block comment in between other elements on the same line is disallowed (cannot be auto-corrected)

Co-authored-by: David Allison <[email protected]>
This prepares for the gradle plugin upgrade
And some for ktlint 1.5.0

after updating gradle.ktlint, the following command now succeeds
`./gradlew --rerun-tasks ktLintFormat`

Process:
* update ktlint in 'libs.versions.toml' to 12.1.1
* run `./gradlew --rerun-tasks ktlintFormat`
* rollback
* run `./gradlew --rerun-tasks ktlintFormat`
* manually fixup remaining issues
@david-allison david-allison added Review High Priority Request for high priority review Lint labels Dec 9, 2024
Bumps org.jlleitschuh.gradle.ktlint from 11.6.1 to 12.1.1.

Then run `./gradlew --rerun-tasks ktlintFormat`

---
updated-dependencies:
- dependency-name: org.jlleitschuh.gradle.ktlint
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
* update dependency
* `./gradlew --rerun-tasks ktLintFormat`
* revert dependency
* commit
Manually fixed up the result of
`./gradlew --rerun-tasks ktLintFormat`

I had issues with `libs.versions.toml`, so this was left as a TODO
This depends on ktlint, which we will want as a dependency
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Wow! Someone's going for the LOC-changed-leaderboard 😆
The whole point of autolinters is not to argue about them, I personally see no point in it
As long as everyone is aware that they will need to rebase their PRs after this - needs a big announcement I think? - works for me
This dependency has hung out for far too long and it really queued up quite the change

@mikehardy mikehardy added the Needs Second Approval Has one approval, one more approval to merge label Dec 9, 2024
@david-allison
Copy link
Member Author

david-allison commented Dec 9, 2024

I do want to note I added:

# We often have block comments representing implementation details after KDocs.
# This feels like an acceptable commenting strategy
ktlint_standard_no-consecutive-comments = disabled

This did not like UniqueArrayList at all (as an example), even though I found the comments to be clean.

/**
* A collection of items that doesn't allow duplicate items, and allows fast random access, lookup, maintaining order, and sorting.
*
*
* The [List] interface makes certain assumptions/requirements. This
* implementation breaks these in certain ways, but this is merely the result of
* rejecting duplicates. Each violation is explained in the method, but it
* should not affect you.
*
* This class does also implement the [Set] interface, and as a result
* you should bear in mind that Sets require immutable objects to function correctly.
*
* @implNote The implementation of this class extends [SetUniqueList] and adds the ability to define a comparator
* to be used to judge uniqueness of elements, and allows sorting.
*
*
* Data structures used internally:
* - [ArrayList] to enable fast random access, sorting, and maintaining order of items during iteration
* - [TreeSet] if a comparator is given or a [HashSet] otherwise.
*/
class UniqueArrayList<E>
/**
* Constructor that wraps (not copies) the List and specifies the set to use.
*
*
* The set and list must both be correctly initialised to the same elements.
*
* @param set the set to decorate, must not be null
* @param list the list to decorate, must not be null
* @throws NullPointerException if set or list is null
*/
private constructor(
/**
* Internal list used in [SetUniqueList] implementation.
*
*
* This is the same list as the one used internally in [SetUniqueList]. We keep a reference to it here in
* order to be able to sort it. [SetUniqueList] implementation needs to make sure the internal [Set]
* and [List] don't get out of sync, and [<] cannot be sorted via [Collections.sort]
* or [SetUniqueList.sort] both will throw an exception, due to a limitation on this class [java.util.ListIterator]
*
* Sorting can be only done via [UniqueArrayList.sort] or [UniqueArrayList.sort].
*
* Modification to this list reference should be done with cautious to avoid having the internal [Set] out of sync
*/
private val list: MutableList<E>,
set: Set<E>?
) : SetUniqueList<E>(list, set), MutableList<E>, MutableSet<E> {
/**
* Constructs a new empty [UniqueArrayList]
*/
constructor() : this(ArrayList<E>(), HashSet<E>())
/**
* Sorts the list into ascending order, according to the
* [natural ordering][Comparable] of its elements.
* All elements in the list must implement the [Comparable]
* interface. Furthermore, all elements in the list must be
* *mutually comparable* (that is, `e1.compareTo(e2)`
* must not throw a `ClassCastException` for any elements
* `e1` and `e2` in the list).
*
* @see .sort
*/
@KotlinCleanup("sortWith")
fun sort() {
sortOverride(null)
}

We lose out on the following, which is a little messy

/*
* List-based utilities
* ***********************************************************
*/
/** {@inheritDoc} */
// this is now a no-op - the tags are canonified when the note is saved
fun canonify(tagList: List<String>): AbstractSet<String> {
// libAnki difference: tagList was returned directly
return HashSet(tagList)
}

But the fix with additional lines was worse:

     /* 
      * List-based utilities 
      * *********************************************************** 
      */ 

     // this is now a no-op - the tags are canonified when the note is saved 

     /** {@inheritDoc}  */   
     fun canonify(tagList: List<String>): AbstractSet<String> { 
         // libAnki difference: tagList was returned directly 
         return HashSet(tagList) 
     } 

Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Thank you

@BrayanDSO BrayanDSO added this pull request to the merge queue Dec 9, 2024
@BrayanDSO BrayanDSO added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Dec 9, 2024
Merged via the queue into ankidroid:main with commit 63e22a2 Dec 9, 2024
9 checks passed
@github-actions github-actions bot added this to the 2.21 release milestone Dec 9, 2024
@github-actions github-actions bot removed Review High Priority Request for high priority review Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) labels Dec 9, 2024
@david-allison david-allison deleted the ktlint-deps-3 branch December 10, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants