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

Each Platforms override HttpLoader's methods #861

Merged
merged 3 commits into from
Apr 21, 2023
Merged

Conversation

iNoles
Copy link
Collaborator

@iNoles iNoles commented Apr 19, 2023

Thanks for the suggestions from #859. Did this meet your approval, @dalewking

@dalewking
Copy link

dalewking commented Apr 20, 2023

That is definitely a step in the right direction.

I see another improvement probably not directly related to my request, but this change makes it easier. I would ask why Fuel class is using expect/actual? Some of the methods don't vary at all across platforms. The only platform specific thing in there is a line like this:

        val loader = httpLoaderFactory?.newHttpLoader() ?: AppleHttpLoader()

But you already have the platform specific mapping in FuelBuilder. It would seem to me that you could remove the expect/actual from the Fuel class and that line could just be:

        val loader = httpLoaderFactory?.newHttpLoader() ?: FuelBuilder().build()

Copy link

@dalewking dalewking left a comment

Choose a reason for hiding this comment

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

Other than my comment about removing expect/actual from Fuel class (which can be handled separately), this was what I was looking for

@dalewking
Copy link

Thinking about it shouldn't FuelBuilder just implement HttpLoaderFactory so you could just pass your your configured FuelBuilder to Fuel.setHttpLoader(factory)?

@iNoles iNoles requested a review from kittinunf April 21, 2023 01:35
Copy link
Owner

@kittinunf kittinunf left a comment

Choose a reason for hiding this comment

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

I also like this at well! Not a big fan of actual/expect class if we can avoid this.

@iNoles
Copy link
Collaborator Author

iNoles commented Apr 21, 2023

I also like this at well! Not a big fan of actual/expect class if we can avoid this.

Trying to avoid it with the old GC for Kotlin Native for new memory management.

@iNoles iNoles merged commit b2f1331 into main Apr 21, 2023
@delete-merged-branch delete-merged-branch bot deleted the httploader_interface branch April 21, 2023 14:14
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.

3 participants