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
Multi-param methods should not qualify for a setter #3002
Conversation
I would suggest a test with
to make sure that the issue isn't with having multiple setters. |
@owengray-google good call, will add it 👍 |
val configuration = dokkaConfiguration { | ||
sourceSets { | ||
sourceSet { | ||
sourceRoots = listOf("src/main/java") | ||
includeNonPublic = true |
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.
It wasn't obvious that the default configuration had this set to true..
it bit me and it's not desired/expected in the majority of the tests, so I made it public-only by default, and added separate local configurations for where private api is needed
@@ -387,35 +386,176 @@ class DefaultPsiToDocumentableTranslatorTest : BaseAbstractTest() { | |||
} | |||
|
|||
@Test | |||
fun `should preserve regular functions that look like accessors, but are not accessors`() { |
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.
I realized that the old test didn't make much sense as it was
If the field is public, no getters and setters are set for it (there's a separate test for that). If the field is private, but the getter doesn't qualify, the setter should stay a regular function (added a separate test for that).
So I refactored this one to check specifically for getter lookalikes as it wasn't covered
@owengray-google could I ask you to review the PR, please? I've added the test cases you suggested (if I got the idea right), as well as some others I could think of on the spot |
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.
Approved!
Fixes #2992
Haven't tested it extensively, but I'm pretty sure that's where the problem is