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

Relax Codable Router type requirements #18

Merged
merged 1 commit into from
May 4, 2018
Merged

Relax Codable Router type requirements #18

merged 1 commit into from
May 4, 2018

Conversation

djones6
Copy link
Contributor

@djones6 djones6 commented Apr 11, 2018

Description

Implementation of Kitura Evolution proposal: Relax Type Constraints for Codable Routing

Resolves Kitura/Kitura#1235 by relaxing the type requirements on the Codable Router to Decodable for input types, and Encodable for output types.

This enables a corresponding Pull Request in Kitura: Kitura/Kitura#1242

As this is merely a relaxing of type constraints, it can be delivered in a patch.

Motivation and Context

See Kitura/Kitura#1235.

The Codable Router was designed with a symmetry in mind for types to be shared between the client and server side (KituraKit <-> Kitura), which requires those types to be both Encodable and Decodable.

In addition, for types that contain purely Codable attributes, conformance to Codable can be synthesized by the compiler. However, there are cases where conformance must be implemented manually.

For cases where the user only wants to use type safety in a single direction (such as Decodable for a type that will only be received), we currently require them to implement conformance to Encodable (at least dummy conformance to satisfy the type requirement).

How Has This Been Tested?

Tests have been added to Kitura in Kitura/Kitura#1242 that are dependent on these changes, and those tests are passing.

Checklist:

  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

@djones6 djones6 changed the title Relax type constraints to Encodable and Decodable Relax Codable Router type requirements Apr 11, 2018
@codecov-io
Copy link

codecov-io commented Apr 11, 2018

Codecov Report

Merging #18 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff          @@
##           master   #18   +/-   ##
====================================
  Coverage      82%   82%           
====================================
  Files           6     6           
  Lines         489   489           
====================================
  Hits          401   401           
  Misses         88    88
Flag Coverage Δ
#KituraContracts 82% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d895952...5d6a299. Read the comment docs.

Copy link
Contributor

@ianpartridge ianpartridge left a comment

Choose a reason for hiding this comment

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

LGTM.

@djones6
Copy link
Contributor Author

djones6 commented Apr 12, 2018

This is not quite ready. Although this is now correctly typed, there is currently no way for a user to POST a Decodable type, because the Codable Router currently requires that an Encodable type be returned in the response.

We need to have a think about how the API should permit this, which may require an additional closure type to be defined - such as allowing just an Id to be returned with no body, or allowing a (nil, nil) or (Identifier, nil, nil) response.

@djones6
Copy link
Contributor Author

djones6 commented May 2, 2018

Kitura/Kitura@d8ebc69 relaxes the requirements for the Codable router result handler to permit a (nil, nil) response.

If this is deemed acceptable, then this PR is ready to go.

@djones6
Copy link
Contributor Author

djones6 commented May 4, 2018

IBM-Swift/evolution#11 has been reviewed and approved.

@djones6 djones6 merged commit 72cbab5 into master May 4, 2018
@djones6 djones6 deleted the issue_1235 branch May 4, 2018 09:53
djones6 added a commit that referenced this pull request May 16, 2018
djones6 added a commit that referenced this pull request May 16, 2018
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.

CodableClosure should use Decodable -> Encodable
3 participants