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 dependent response objects for business endpoint #4

Merged
merged 12 commits into from
Feb 17, 2016

Conversation

d9chen
Copy link
Contributor

@d9chen d9chen commented Jan 22, 2016

@SSheldon This PR adds in the rest of the response objects needed by YLPBusiness. I've also got the 'private_header_files' line commented out for now.

Xcode didn't seem to like that I was importing these headers in my tests which are targeted against the YelpAPI example project as opposed to the Pods project. Initially, the tests were targeted against the Pods project, but as I was git ignore the Pods/* dir the scheme and targets didn't carry through when the repo was checked out. My solution was to make my pod tests target the YelpAPI project instead, which seems to allow the scheme and target carry through when the repo is checked out.

@@ -5,7 +5,7 @@
// Created by David Chen on 1/8/16.
//
//
#import "YLPClient.h"
@class YLPCLient;
Copy link
Member

Choose a reason for hiding this comment

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

This is surprising to me, I'd assume that you must actually have the class imported to declare an extension to it. I'm wondering if this only works because you happen to have imported YLPClient beforehand in all cases before you import this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm interesting. I wasn't paying much attention when I changed this over to class. Thinking about it now it is weird that this is working. Switching back to an import.

@SSheldon
Copy link
Member

Hmm, not sure about the issues you were running into with the private headers, I'll check out the project sometime and look at it.

@SSheldon
Copy link
Member

SSheldon commented Feb 9, 2016

Ahh sorry @d9chen, GitHub doesn't notify me when you push new commits so I had no idea this had been updated :( Will take a look soon.

@kanexren
Copy link

@SSheldon thanks. :) ❤️

#import <YelpAPI/YLPDealOption.h>
#import <YelpAPI/YLPLocation.h>
#import <YelpAPI/YLPReview.h>
#import <YelpAPI/YLPUser.h>
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we should probably add an umbrella header at some point. That'd be one header, YelpAPI.h, which will import all these things for us.

Then, users of the pod can just do #import <YelpAPI/YelpAPI.h> without having to import each individual thing.

Copy link
Member

Choose a reason for hiding this comment

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

I don't actually know how umbrella headers work in pods, it seems that one has already been generated but I can't actually import it.

Anyways, probably not something that needs to be fixed now.

@SSheldon
Copy link
Member

Code looks perfect, ship it! :shipit:

Only things I was curious about is the pod organization and structure, but I think we can always tweak that later, so don't let that block you. I don't know all the answers there, haven't made many pods myself.

d9chen added a commit that referenced this pull request Feb 17, 2016
…point

Add dependent response objects for business endpoint
@d9chen d9chen merged commit ea97628 into master Feb 17, 2016
@d9chen d9chen deleted the add_response_objects_for_business_endpoint branch April 21, 2016 23:23
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

5 participants