-
-
Notifications
You must be signed in to change notification settings - Fork 755
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
Split UnusedPrivateMember #5722
Merged
Merged
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
968683f
Run ktlint --format on files about to be touched
drawers 7cb6c0f
Make UnusedPrivateMember focus on private functions
drawers 806a415
Extract UnusedParameter
drawers 429142c
Extract UnusedParameter
drawers 09a7c6b
Extract UnusedPrivateProperty
drawers 7c50787
Move first batch of property tests from inside UnusedPrivateMemberSpec
drawers eb11585
Move second batch of property tests from inside UnusedPrivateMemberSpec
drawers a86e950
Readjust expectations in test now that subject only detects unused pr…
drawers 7ff9c21
Move over next batch of property tests from UnusedPrivateMemberSpec
drawers 39491ae
Move one test from "unused class declarations which are allowed"
drawers 49df7cb
Move "nested class declarations" from UnusedPrivateMemberSpec
drawers 939c679
Split "error messages"
drawers f6ffe46
Move "suppress unused property warning annotations"
drawers 2406a24
Move "main methods" inner test class
drawers 8048a00
Move "backtick identifiers" inner test class
drawers 4376627
Move "backtick identifiers" inner test class for unused private param…
drawers 93f090d
Split "highlights declaration name"
drawers 99a350b
Move "parameter with the same name as named argument"
drawers 636b3ad
Split "actual functions and classes"
drawers 18830f8
Move "properties in primary constructors"
drawers 5227986
Fix typo
drawers 9241ae4
Fix disabled tests
drawers 8c4c509
Add alias "UnusedPrivateMember" to UnusedParameter and UnusedPrivateP…
drawers b563ece
Rename "UnusedPrivateParameterSpec" to "UnusedParameterSpec"
drawers 5a3a901
Adjust KDoc to say "source file" instead of "class etc." for clarity
drawers 5897a31
Adjust KDoc to explain how UnusedPrivateProperty can detect unused co…
drawers 1e4bcb8
inline method
chao2zhang 8f9a9e5
Merge branch 'main' into fix-3418-split
schalkms ddd19eb
Remove duplicate `main methods` inner class after merge
drawers File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
You are viewing a condensed version of this merge commit. You can view the full changes here.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @schalkms thank you for performing the merge 🙏 but I think this
main methods
now needs to be inUnusedParameterSpec.kt
rather than inUnusedPrivateMemberSpec.kt
. That was how it was in the original patch I submitted since those test cases now coverUnusedParameter.kt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schalkms would you prefer me to add a commit to address this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Btw, sorry for messing around with the merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drawers you were a few seconds faster. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schalkms I added a commit ddd19eb to address this. If I take Chao's last commit before the merge as authoritative (1e4bcb8) then I do:
git diff --name-only 1e4bcb8 | grep Unused
I get:
Then if I do:
I get:
So we end up with only the change from renaming
inner class operators
toinner class Operators
and nothing else. Which I think is what we want.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also rollback the merge and do a rebase. That might be easier to grasp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schalkms yes if you would prefer me to rebase then I don't mind doing it just let me know 👍
(sorry about leaving the branch to go stale, I didn't anticipate another PR touching that test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drawers if we are good to go after the merge, we are fine, no need to do another rebase then, IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schalkms looks okay to me now!