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

Data parser associated types #230

Merged
merged 2 commits into from
Mar 12, 2017

Conversation

gormster
Copy link

DataParser should not be required to return Any and have objects checked for type safety at runtime. This PR allows us to use the Swift build time type checking system with very minimal changes to existing code. The only API breaking change is that Request classes using the default JSONDataParser should now conform to JSONRequest, instead of just Request. This is because protocol extensions cannot declare default associated types.

How was this not already there?
DataParser should not be required to return Any and have objects checked for type safety at runtime. This allows us to use the Swift build time type checking system with very minimal changes to existing code. The only API breaking change is that Request classes using the default JSONDataParser should now conform to JSONRequest, instead of just Request. This is because protocol extensions cannot declare default associated types.
@gormster
Copy link
Author

You could also, of course, avoid API breaking changes by renaming Request to BaseRequest and JSONRequest to Request. Not sure which of those you'd prefer.

@ishkawa
Copy link
Owner

ishkawa commented Mar 12, 2017

Thank you for this PR!

DataParser should not be required to return Any and have objects checked for type safety at runtime. This PR allows us to use the Swift build time type checking system

Right. The intermediate Any is introduced to express JSON object and array, but it requires casting in intercept(object:urlResponse:) even if the data parser returned non-Any type. Your PR resolves this issue 👍

This PR contains breaking change, I'm going to merge this into a develop branch for the next major version.

@ishkawa ishkawa changed the base branch from master to develop/4.0 March 12, 2017 05:07
@ishkawa ishkawa merged commit 5f67418 into ishkawa:develop/4.0 Mar 12, 2017
@Econa77 Econa77 mentioned this pull request Aug 13, 2022
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.

None yet

2 participants