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

Bridge KituraNet.HTTPStatusCode #1217

Merged
merged 13 commits into from Mar 16, 2018
Merged

Bridge KituraNet.HTTPStatusCode #1217

merged 13 commits into from Mar 16, 2018

Conversation

tunniclm
Copy link
Collaborator

@tunniclm tunniclm commented Feb 6, 2018

Description

Add a typealias of HTTPStatusCode into Kitura so that a user can use the type without also having to import KituraNet.

Motivation and Context

While developing with Kitura, @CosmicYogi ran into unresolved symbol errors with HTTPStatusCode. Thus led to a discussion on Slack and this PR: #1208.

The issue is that you need to import KituraNet if your code uses HTTPStatusCode directly.

The solution proposed by @CosmicYogi was to document the requirement in the related Kitura API documentation. This is a reasonable approach given most people will probably look at the API doc for a function if they see a compile error.

What I think would be even better, is to prevent this issue entirely so that HTTPStatusCode can be used even if only Kitura were imported.

How Has This Been Tested?

Tested on macOS (in SPM and Xcode builds) and Linux, using a simple project that uses HTTPStatusCode and has separate test files that:

  • imports Kitura only
  • imports both Kitura and KituraNet

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 Feb 6, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1217   +/-   ##
=======================================
  Coverage   89.39%   89.39%           
=======================================
  Files          38       38           
  Lines        2234     2234           
=======================================
  Hits         1997     1997           
  Misses        237      237
Flag Coverage Δ
#Kitura 89.39% <ø> (ø) ⬆️

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 62d2183...a814454. Read the comment docs.

Copy link
Collaborator

@djones6 djones6 left a comment

Choose a reason for hiding this comment

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

This looks good. Couple of questions:

  • how does the API documentation look: if someone were to browse the API doc for Kitura.HTTPStatusCode, do they automatically get a reference / link to KituraNet.HTTPStatusCode?
  • does XCode completion 'follow' the typealias and give you the possible values?

Copy link
Contributor

@helenmasters helenmasters left a comment

Choose a reason for hiding this comment

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

The changes look much more intuitive for users.

Like Dave, I think it would be good if we could make the API reference redirect users to the HTTPStatusCode documentation in Kitura-net, but I don't know how feasible this is in jazzy.

@tunniclm
Copy link
Collaborator Author

I tried generating Jazzy for this, but I didn't get any entry for the typealias at all. Not sure why.

@tunniclm
Copy link
Collaborator Author

Fixed the doc generation problem (needed to add a private dummy class to keep Jazzy happy). I have added links to KituraNet.HTTPStatusCode as well.

@tunniclm
Copy link
Collaborator Author

Just waiting until we have the automatic doc generation code wired up before merging this (Kitura/Package-Builder#128)

@djones6 djones6 added the jazzy-doc When applied to a PR, instructs Package-Builder to regenerate the Jazzy documentation label Mar 7, 2018
@CLAassistant
Copy link

CLAassistant commented Mar 16, 2018

CLA assistant check
All committers have signed the CLA.

@djones6
Copy link
Collaborator

djones6 commented Mar 16, 2018

CI for ade5187 will fail until Kitura/Package-Builder#133 is merged.

@djones6
Copy link
Collaborator

djones6 commented Mar 16, 2018

Github does not seem to receive the Travis CI status for a PR when the most recent commit is labelled [ci skip]: travis-ci/travis-ci#2159
@ianpartridge I think we should still merge the PR as Travis for the most recent non-documentation commit (ade5187) has passed. What do you think?

@djones6 djones6 merged commit 6690f2b into master Mar 16, 2018
@djones6 djones6 deleted the bridge_httpstatuscode branch March 16, 2018 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation jazzy-doc When applied to a PR, instructs Package-Builder to regenerate the Jazzy documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants