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: Fix parsing of the API response #850

Merged
merged 9 commits into from
Apr 29, 2024
Merged

fix: Fix parsing of the API response #850

merged 9 commits into from
Apr 29, 2024

Conversation

borisprimer
Copy link
Contributor

This PR fixes a parsing problem for the only case where we have a pure string as a response (HTML). This is mostly for @jnewc since you were working on the API layer refactor. Make sure I did everything as you would do it and feel free to change the implementation if you don't like it.

We are currently waiting for confirmation from Lime that this is working (they are testing from the branch), if we get it while I am away, please make a new release with this fix.

Thanks!

PS: If you need more context, please ping Semir.

Copy link
Contributor

Copy link
Contributor

@jnewc jnewc left a comment

Choose a reason for hiding this comment

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

I don't think this is the right approach, we shouldn't be putting non-generic code into the network service. Especially given that String is a Codable.

It also doesn't appear that we use the string response anywhere, in which case we shouldn't try to decode it as a String and attempt to use an empty response instead, so that endpoint can be changed to not send HTML in the future (as we always want parsable data from APIs)

@borisprimer I'll rework this while you're away.

}
}

class SuccessResponseFactory: NetworkResponseFactory {
Copy link
Contributor

Choose a reason for hiding this comment

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

@NQuinn27 appreciate thoughts on naming here.

The idea is that this will always provide an empty success model because success is implicit based on a 200 code, and the actual response contents are ignored.

ImplicitSuccessResponseFactory or AutoSuccessResponseFactory maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ImplicitSuccessResponseFactory is best, but I dont have very strong feelings

@jnewc jnewc requested review from jnewc and removed request for jnewc April 29, 2024 09:57
@jnewc jnewc merged commit 6de9db0 into master Apr 29, 2024
7 checks passed
@jnewc jnewc deleted the bn/nol_fixes branch April 29, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants