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

Add iOS framework target #36

Closed
wants to merge 3 commits into from
Closed

Add iOS framework target #36

wants to merge 3 commits into from

Conversation

hzalaz
Copy link
Contributor

@hzalaz hzalaz commented Jul 18, 2016

iOS framework target that allows lib to be included using Carthage.


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Jul 18, 2016

Codecov Report

Merging #36 into master will decrease coverage by 1.54%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
- Coverage   76.05%   74.51%   -1.55%     
==========================================
  Files          39       33       -6     
  Lines        2360     1891     -469     
  Branches      123      100      -23     
==========================================
- Hits         1795     1409     -386     
+ Misses        508      437      -71     
+ Partials       57       45      -12
Impacted Files Coverage Δ
Source/OIDServiceConfiguration.h 66.66% <0%> (-8.34%)
Source/OIDAuthState.m 35.71% <0%> (-6.45%)
Source/OIDURLQueryComponent.m 75.32% <0%> (-5.2%)
Source/OIDFieldMapping.m 74.35% <0%> (-2.73%)
Source/OIDAuthorizationRequest.m 75.53% <0%> (-2.57%)
Source/OIDTokenRequest.m 60.63% <0%> (-0.75%)
Source/OIDTokenResponse.m 70.17% <0%> (-0.42%)
UnitTests/OIDTokenRequestTests.m 67.39% <0%> (-0.29%)
Source/OIDFieldMapping.h 100% <0%> (ø)
UnitTests/OIDGrantTypesTests.m 100% <0%> (ø)
... and 27 more

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 ac2b774...c48687c. Read the comment docs.

@hzalaz
Copy link
Contributor Author

hzalaz commented Jul 18, 2016

This closes #25. Also not sure if you guys need an example in the repo

@clayellis
Copy link

@hzalaz @WilliamDenniss Any updates on the review above? Carthage support is a must for growth in the iOS community.

@StevenEWright
Copy link
Collaborator

Reviewed 1 of 6 files at r1.
Review status: 1 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Source/Framework/AppAuth.h, line 1 at r1 (raw file):

/*! @file AppAuth.h

I feel we should be using the same umbrella header that already exists, otherwise we will have to keep multiple umbrella headers in sync?


Comments from Reviewable

@hzalaz
Copy link
Contributor Author

hzalaz commented Dec 6, 2016

Review status: 1 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Source/Framework/AppAuth.h, line 1 at r1 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

I feel we should be using the same umbrella header that already exists, otherwise we will have to keep multiple umbrella headers in sync?

Yes, it's not ideal but I think the framework umbrella header should use the <> import style but will check it during the weekend to see if I can get around this.


Comments from Reviewable

@StevenEWright
Copy link
Collaborator

Review status: 1 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Source/Framework/AppAuth.h, line 1 at r1 (raw file):

Previously, hzalaz (Hernan Zalazar) wrote…

Yes, it's not ideal but I think the framework umbrella header should use the <> import style but will check it during the weekend to see if I can get around this.

Ah, yes, I see, and you are right. I didn't realize the existing umbrella header wasn't referencing the files that way. Let me chat with William and see what's up. Do you have a suggestion in the meantime?


Comments from Reviewable

@StevenEWright
Copy link
Collaborator

@WilliamDenniss PTAL

@sunenielsen
Copy link

Did this request die, or are someone still working on it? Not having Carthage support is really annoying when we were finally about to replace cocoa pods with something a lot better.

@WilliamDenniss
Copy link
Member

I'd love to see Carthage support.

This PR needs to be updated with new classes that were added, and I'd like to see an example project in Examples/ that uses Carthage, that we can use for testing (including CI) & documentation.

If someone can do that work to refresh the PR, I'll make sure this gets reviewed ASAP.

@hzalaz
Copy link
Contributor Author

hzalaz commented Mar 8, 2017

@WilliamDenniss will take care of that

@hzalaz
Copy link
Contributor Author

hzalaz commented Mar 12, 2017

@WilliamDenniss updated the PR with latest master changes and also added targets for tvOS & macOS fwk besides iOS.

Also added a new example project using Carthage, it's configured to pull the dependency from this repo for when its merged but if you need to test you need to change the Cartfile to use my fork github "hzalaz/AppAuth-iOS" "master".

Is there any way I can see why the build failed?

@WilliamDenniss
Copy link
Member

Thanks for all the work @hzalaz!

There were massive merge conflicts with the XCodeProj, so I ended up creating the Framework targets again from scratch. I also added test targets for each framework. Updated PR #97 adds the framework, and cherry-picks your example & documentation commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants