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

Typealias RequestError to avoid KituraContracts import #1267

Merged
merged 4 commits into from May 11, 2018

Conversation

djones6
Copy link
Collaborator

@djones6 djones6 commented May 10, 2018

Description

In a similar vein to #1217, this adds a typealias RequestError to Kitura, to avoid a user having to explicitly import KituraContracts when defining codable routes.

Motivation and Context

While creating a minimal code example of use of codable routing, @helenmasters noted that we need to import KituraContracts because of the RequestError? in the declaration of signature for a codable handler. This allows us to skip that import.

There are other situations where it may still be necessary to import KituraContracts, such as when defining a custom Identifier type, however the import can be confined to the source file that defines the type.

How Has This Been Tested?

A simple example of codable routing now compiles without importing KituraContracts.

The Kitura tests passed locally.

Checklist:

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

@codecov-io
Copy link

codecov-io commented May 10, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1267   +/-   ##
=======================================
  Coverage   88.15%   88.15%           
=======================================
  Files          37       37           
  Lines        2355     2355           
=======================================
  Hits         2076     2076           
  Misses        279      279
Flag Coverage Δ
#Kitura 88.15% <ø> (ø) ⬆️
Impacted Files Coverage Δ
Sources/Kitura/CodableRouter.swift 81.31% <ø> (ø) ⬆️

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 91f47de...4d1ad9d. Read the comment docs.

@ianpartridge
Copy link
Collaborator

#1217 included a test, is that possible here?

Copy link
Collaborator

@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.

Looks fine in principle, would like a test.

@@ -19,6 +19,11 @@ import LoggerAPI
import KituraNet
import KituraContracts

/// Bridge [RequestError](https://ibm-swift.github.io/KituraContracts/Structs/RequestError.html)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell, this dummy class was only necessary because the file otherwise contains only a typealias. In this case I dropped the typealias into the existing CodableRouter.swift since I could argue it belongs there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK as long as Jazzy is generating the typealias OK then we're fine :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tested locally, it generates fine.

And also enable the existing bridging test
@djones6
Copy link
Collaborator Author

djones6 commented May 11, 2018

Hmm, that's interesting, my test fails to compile on Swift 4.0.3. It appears I inadvertently used some Swift 4.1 feature.

@djones6 djones6 merged commit 991aa2a into master May 11, 2018
@djones6 djones6 deleted the typealias_RequestError branch May 11, 2018 13:36
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

3 participants