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

Remove 'by' property setter helper function #2706

Closed
wants to merge 15 commits into from

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Oct 11, 2022

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

@IgnatBeresnev IgnatBeresnev added the runner: Gradle plugin An issue/PR related to Dokka's Gradle plugin label Oct 12, 2022
@IgnatBeresnev
Copy link
Member

IgnatBeresnev commented Oct 12, 2022

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 IgnatBeresnev self-requested a review October 12, 2022 20:48
@Goooler
Copy link
Contributor

Goooler commented Oct 13, 2022

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

@IgnatBeresnev
Copy link
Member

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

@Goooler
Copy link
Contributor

Goooler commented Oct 13, 2022

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

@aSemy
Copy link
Contributor Author

aSemy commented Oct 13, 2022

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

@IgnatBeresnev
Copy link
Member

IgnatBeresnev commented Jan 29, 2023

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

@aSemy
Copy link
Contributor Author

aSemy commented Jan 29, 2023

Moved to new independent PR: #2834

@aSemy aSemy closed this Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runner: Gradle plugin An issue/PR related to Dokka's Gradle plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants