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

fix: Removed "it" from forbiddenKeywords and replace "" with "_ -> " … #162

Conversation

FCatinella
Copy link
Contributor

Adding back ticks to "it" causes problems when ksp tries to generate variable names when package names start with "it", hence the removal from forbiddenKeywords.

We (samuelepozzebon and I) also changed "" to "_ -> " in order to fix the issue "it" creates since it is used as the default param name.

For example:
single() { 'it'.example.italy.classProvider() }...
become
single() { _ -> it.example.italy.classProvider() }...

This PR fixes: #161

…when generating params string in case of no injected params. Fixes InsertKoinIOgh-161 (koin-annotations)
@arnaudgiuliani arnaudgiuliani self-requested a review September 17, 2024 14:59
@arnaudgiuliani arnaudgiuliani modified the milestone: 1.4.0 Sep 17, 2024
@arnaudgiuliani
Copy link
Member

Ok, looking at it

@arnaudgiuliani arnaudgiuliani added the status:checking Ticket is currently being checked label Sep 17, 2024
@arnaudgiuliani
Copy link
Member

We (samuelepozzebon and I) also changed "" to "_ -> " in order to fix the issue "it" creates since it is used as the default param name.

I don't see the problem in that case? We don't use it on generated code definition 🤔

Can you add a test case?

@arnaudgiuliani arnaudgiuliani added status:waiting_feedback and removed status:checking Ticket is currently being checked labels Sep 17, 2024
@FCatinella
Copy link
Contributor Author

Hi! Sorry for not being clear. What I meant is that Lambas in Kotlin use "it" as default parameter name when not specified.
Hence the explicit parameters declaration "_ ->" instead of ""

Btw, I'll add a test case asap.
Thanks for your time

@arnaudgiuliani
Copy link
Member

I'll merge your PR and add somethign simple to test 👍

@arnaudgiuliani arnaudgiuliani merged commit f60f14c into InsertKoinIO:1.4.0 Oct 15, 2024
1 check passed
@arnaudgiuliani
Copy link
Member

thanks @FCatinella

@FCatinella
Copy link
Contributor Author

Thanks to you @arnaudgiuliani

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants