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

Migrate Geocoder to coroutines #344

Merged
merged 9 commits into from
Apr 11, 2022
Merged

Migrate Geocoder to coroutines #344

merged 9 commits into from
Apr 11, 2022

Conversation

ferranpons
Copy link
Member

@ferranpons ferranpons commented Apr 8, 2022

Description

In order to migrate all the library from RxJava to Kotlin Coroutines we have migrated the GeocoderRepository to Coroutines.

Screenshots

No screens

Mandatory GIF

mandatory

@ferranpons ferranpons added the wip label Apr 8, 2022
@ferranpons ferranpons self-assigned this Apr 8, 2022
@ferranpons ferranpons added review and removed wip labels Apr 8, 2022
@ferranpons ferranpons marked this pull request as ready for review April 8, 2022 10:29
} catch (exception: IOException) {
emptyList()
}
override fun getFromLocationName(query: String): List<Address> {

Choose a reason for hiding this comment

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

geocoder.getFromLocationName(query, MAX_RESULTS) can block the thread? should getFromLocationName be a suspend fun?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about this, I think It should not be a problem because is always called under a suspend fun. And I also think that has a time out exception that raises the IOException.

.build()
val results = geoDataClient.findAutocompletePredictions(findAutocompletePredictionsRequest)
try {
Tasks.await(results, PREDICTIONS_WAITING_TIME, TimeUnit.SECONDS)

Choose a reason for hiding this comment

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

What are you trying to achieve here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here I only removed the Single.defer wrap. If I remember well this was a copy&paste from Google documentation.
🤔 Probably this must be reviewed later on.

Copy link

@danieldisu danieldisu left a comment

Choose a reason for hiding this comment

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

👍 with non-blocking comments

@ferranpons ferranpons merged commit 3937fc3 into master Apr 11, 2022
@delete-merged-branch delete-merged-branch bot deleted the migrate-to-coroutines branch April 11, 2022 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants