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

Improve calling addListener #4244

Merged
merged 1 commit into from
Jun 27, 2023
Merged

Improve calling addListener #4244

merged 1 commit into from
Jun 27, 2023

Conversation

hfhbd
Copy link
Collaborator

@hfhbd hfhbd commented Jun 7, 2023

Fixes #4237

I also changed queryKeys: Array<String> to vararg queryKeys: String, because I think, this api is nicer as developer.

@@ -27,12 +27,12 @@ import app.cash.sqldelight.db.SqlDriver
@Suppress("FunctionName") // Emulating a constructor.
fun <RowType : Any> Query(
identifier: Int,
queryKeys: Array<String>,
vararg queryKeys: String,
Copy link
Collaborator Author

@hfhbd hfhbd Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to change this api too? This aligns with the addListener methods, but requires a lot of changes due to required named parameters.

@dellisd
Copy link
Collaborator

dellisd commented Jun 7, 2023

Given that we've now pushed out a release candidate, the intent is to keep the API frozen. This is a breaking change so I think the ship has sailed on this for 2.0.

Can this be added as an extension method instead?

@hfhbd
Copy link
Collaborator Author

hfhbd commented Jun 7, 2023

We don’t accept any breaking changes? We still do have some open issues marked with milestone 2.0 which will break the current api.

@vanniktech
Copy link
Contributor

If you change it to vararg, we can't leverage the lambda:

Screenshot 2023-06-08 at 10 53 50

I'd keep it as Array<String>.

@hfhbd
Copy link
Collaborator Author

hfhbd commented Jun 8, 2023

It works, see #4244.

fun test(vararg test: String, foo: Foo)

@vanniktech
Copy link
Contributor

Ah my bad, I named the parameter vararg 😅

Copy link
Collaborator

@AlecKazakova AlecKazakova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yolo

@AlecKazakova AlecKazakova merged commit b445448 into master Jun 27, 2023
5 checks passed
@AlecKazakova AlecKazakova deleted the hfhbd/addListener branch June 27, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change parameter positions for add / remove listener
4 participants