vicpinm / Kotlin-Realm-Extensions

Kotlin extensions to simplify Realm API.
Apache License 2.0
535 stars 53 forks source link

Enhancement: Allow parameterized query calls. #32

Closed PrashamTrivedi closed 6 years ago

PrashamTrivedi commented 6 years ago

Currently to query, I have to write something like this val events = Event().query { query -> query.equalTo("id",1) }

Which according to me

I think the better way of writing query functions should be something like this

val events = query<Event> {query -> query.equalTo("id", 1) }

This feels more idiomatic and looks memory friendly.

PrashamTrivedi commented 6 years ago

Update: As a proof of concept I forked the repo and did the changes that matches the code I like to write. The changes are

Also some additional changes in the fork

The current state of work is just Proof of concept, I would like to have your feedback on how you want to proceed with it.

vicpinm commented 6 years ago

Hi @PrashamTrivedi, it's true that you have to create an empty object to perform the query, but unless your app performs a huge amount of queries or has some strict memory requirements, I think the memory consumed by this empty object is ridiculous. My first aim was to create a static extension, like Event.query, but this is not possible currently with Kotlin. Your solution sounds good, although I don't see why is more idiomatic than the current solution, but is definitely more memory friendly.

If you make a PR, I will be pleased to merge it into the project. However, please take into account the following:

Thank you so much for your contribution!

PrashamTrivedi commented 6 years ago

Please respect the current code style. It's difficult to see the changes because your indentation is different, and a lot of lines have changed.

It would be helpful if there is a code style settings in the project. I use this approach, I welcome codestyle settings anyway you prefer, the approach mentioned above or a jar export.

In T.query method, I'm not sure why you have changed it into two lines.

I left it there for some debugging, won't go in final PR.

vicpinm commented 6 years ago

I'm using the default code style from Android studio. I attach the file with the rules (please rename the extension to xml). Default.xml.txt

vicpinm commented 6 years ago

Hi @PrashamTrivedi, I'm not sure if you are finally going to send a PR. If not, I will try to make time to implement these changes.

magillus commented 6 years ago

@vicpinm I was was doing something similar at some point: https://gist.github.com/magillus/c0795671c10d191001d1f61d9db66ec5 - RxRealmQueryFlowable.kt

PrashamTrivedi commented 6 years ago

Hi. Got stuck between family metters and bad health. The work is not abandoned. In fact main work is completed, only tests are remaining. I am planning to complete it by tomorrow....

On Thu 4 Jan, 2018, 10:43 PM Mateusz Perlak, notifications@github.com wrote:

@vicpinm https://github.com/vicpinm I was was doing something similar at some point: https://gist.github.com/magillus/c0795671c10d191001d1f61d9db66ec5 - RxRealmQueryFlowable.kt

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vicpinm/Kotlin-Realm-Extensions/issues/32#issuecomment-355341542, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7WhmzkPqbcLDU3pvhzdw-wcLrlcAMrks5tHQbJgaJpZM4RAMGV .

--

Prasham H Trivedi

vicpinm commented 6 years ago

Hi @PrashamTrivedi, don't you worry, take your time, there is no rush. @magillus your way is other option to consider. When Prasham sends me the PR, I will try to compare both options and decide which is simpler and cleaner, or maybe implement a third option with a mixture of both, who knows.

PrashamTrivedi commented 6 years ago

Hi @vicpinm Just submitted #37 . Added some points there for context...

vicpinm commented 6 years ago

Thanks your very much @PrashamTrivedi , I will review it as soon as I can.

vicpinm commented 6 years ago

The changes are now merged in master.