dokka icon indicating copy to clipboard operation
dokka copied to clipboard

Remove 'by' property setter helper function

Open aSemy opened this issue 1 year ago • 5 comments

Part of #2700

I found these extension functions very confusing. I'm not sure what their purpose is. Do they only rename an existing function?

It's confusing because 'by' is already a Kotlin keyword for delegation, but these extension functions don't do anything with delegation. I think using the normal .set() function makes the code more readable and understandable because .set() is a common Gradle function.

This PR includes changes from

  • #2702
  • #2701
  • #2705

aSemy avatar Oct 11 '22 20:10 aSemy

Not sure why these methods were introduced (the PR itself is huge, #1194), but I agree that it can be confused with Kotlin's by keyword, and overall makes the code a little more difficult to understand.

I'm ready to approve it once it has been rebased and contains no changes from other PRs, so let's wait for the other ones to be merged/closed first

IgnatBeresnev avatar Oct 12 '22 20:10 IgnatBeresnev

Use by to set values in Kotlin dsl is reasonable, no need to replace it.

Goooler avatar Oct 13 '22 10:10 Goooler

Use by to set values in Kotlin dsl is reasonable, no need to replace it.

That's not part of the DSL though, and I honestly fail to see what value it brings. If it's considered to be a good practice, could you link to examples/docs/etc?

by makes me think it's a delegate of some sort, or that there's some complicated logic with how the value is assigned or computed, whereas it's a simple set

IgnatBeresnev avatar Oct 13 '22 11:10 IgnatBeresnev

Alright, I haven't found strong evidence for using by in setting properties, it might due to my false memory.

Goooler avatar Oct 13 '22 11:10 Goooler

Alright, I haven't found strong evidence for using by in setting properties, it might due to my false memory.

Are you thinking about how you can do val someTask by tasks.registering { }? https://docs.gradle.org/current/userguide/kotlin_dsl.html#using_kotlin_delegated_properties

aSemy avatar Oct 13 '22 11:10 aSemy

I'd be happy to merge it now if the changes don't include the postponed #2702 and #2705

IgnatBeresnev avatar Jan 29 '23 13:01 IgnatBeresnev

Moved to new independent PR: https://github.com/Kotlin/dokka/pull/2834/

aSemy avatar Jan 29 '23 15:01 aSemy