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

Conversation

Projects
None yet
7 participants
@tunniclm
Collaborator

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

This comment has been minimized.

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.

@tunniclm tunniclm requested review from djones6 and helenmasters Feb 6, 2018

@djones6

djones6 approved these changes Feb 8, 2018 edited

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?
@helenmasters

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

This comment has been minimized.

Collaborator

tunniclm commented Feb 19, 2018

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

@tunniclm

This comment has been minimized.

Collaborator

tunniclm commented Feb 20, 2018

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

This comment has been minimized.

Collaborator

tunniclm commented Feb 27, 2018

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

tunniclm and others added some commits Feb 27, 2018

@djones6 djones6 added the jazzy-doc label Mar 7, 2018

@djones6 djones6 referenced this pull request Mar 8, 2018

Merged

Fixes for auto jazzy doc logic #130

0 of 3 tasks complete
@CLAassistant

This comment has been minimized.

CLAassistant commented Mar 16, 2018

CLA assistant check
All committers have signed the CLA.

@djones6 djones6 referenced this pull request Mar 16, 2018

Merged

Complete jazzy-doc auto documentation feature #133

3 of 3 tasks complete
@djones6

This comment has been minimized.

Member

djones6 commented Mar 16, 2018

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

@djones6

This comment has been minimized.

Member

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

1 check passed

license/cla Contributor License Agreement is signed.
Details

@djones6 djones6 deleted the bridge_httpstatuscode branch Mar 16, 2018

@djones6 djones6 referenced this pull request Mar 16, 2018

Closed

Added documentation comment related to HTTPStatusCode #1208

3 of 3 tasks complete

@djones6 djones6 referenced this pull request May 10, 2018

Merged

Typealias RequestError to avoid KituraContracts import #1267

1 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment