Skip to content
This repository was archived by the owner on Jan 16, 2021. It is now read-only.

Add support for using PF_Twitter without a linking or logging into a Parse account #27

Merged
merged 4 commits into from
Apr 28, 2016
Merged

Conversation

zadr
Copy link
Contributor

@zadr zadr commented Apr 27, 2016

There are two things going on here, in two separate commits:

  1. On 16121e2, expose/document how -[PFTwitter init] works in that it is okay to call, and after setting consumerKey and consumerSecret, can be used to make Twitter API calls.
  2. In 6e5e9ca, add support for deauthorization, by calling oauth2/invalidate_token on Twitter's API, and then to clear out local state on success.

6e5e9ca contains tests for the newly added deauthorizeInBackground call, however, I wasn't able to get the tests to run locally, so, I can't promise that the tests do the right thing (after running bundle install locally, updating the deployment target, enabling ARC in the project and a few other steps that have slipped my mind, the rabbit hole seemed too deep to keep following).

Went to sign the CLA, and it told me The 'Email' value has been registered on this form before. If you would like to update an existing contributor account, please email [email protected] with your details., so, I think thats all taken care of.

Zachary Drayer added 2 commits April 27, 2016 12:51
It is useful to be able to create a standalone instance of `PFTwitter` to make authenticated Twitter API calls without having to link a Twitter account to a Parse account or require Twitter login.
@@ -22,6 +22,15 @@ NS_ASSUME_NONNULL_BEGIN
@interface PF_Twitter : NSObject

/**
An instance of `PF_Twitter` configured to access device Twitter accounts with Accounts.framework,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Initializes an instance of ...

@nlutsenko
Copy link
Contributor

Hey @zadr, this is amazing and almost flawless.
Most of the requested changes are small nits, but the most important part is actually to fix tests, since we don't want to merge in something that fails to build :)

I am not sure on why OCMock doesn't like that expectation and doesn't want to mock it, so this would need a little bit of debugging probably. If you can't make it work though - you can simply create an issue for it to implement it later and I'll be happy to jump on it at any given point.
The other approach that we can take is I could patch it locally and see what I can dig up. Let me know which route would be more helpful for you, so we can unblock it and merge it in!

Overall, once again, this looks great and is an amazing idea to implement this!

@zadr
Copy link
Contributor Author

zadr commented Apr 28, 2016

@nlutsenko thanks for 👀ing this so quickly and thoroughly! added a new commit that addresses all your nits, and, should make all the tests pass.

Using Xcode 7.3, what I had to do to get the tests run (and see what was failing) by: explicitly set the deployment target of the xcodeproj to iphoneos, and then running the following command:

xcodebuild test -workspace ParseTwitterUtils.xcworkspace -sdk iphonesimulator -scheme ParseTwitterUtils-iOS -configuration Debug GCC_INSTRUMENT_PROGRAM_FLOW_ARCS=YES GCC_GENERATE_TEST_COVERAGE_FILES=YES CLANG_ENABLE_OBJC_ARC=YES

Which differs from the command Travis uses to run tests on Xcode 7.2.1:

xcodebuild test -workspace ParseTwitterUtils.xcworkspace -sdk iphonesimulator -scheme ParseTwitterUtils-iOS -configuration Debug -destination "platform=iOS Simulator,name=iPhone 4s" -destination "platform=iOS Simulator,name=iPhone 6s" GCC_INSTRUMENT_PROGRAM_FLOW_ARCS=YES GCC_GENERATE_TEST_COVERAGE_FILES=YES

in that it doesn't specify the -destination, and adds CLANG_ENABLE_OBJC_ARC=YES at the end.

Earlier, on another machine, I wasn't able to get the same steps running with Xcode 7.3.1, so, I guess, Apple changed things around again?

Let me know if you want me to rebase the commit with the PR feedback onto the commit that does most of the work!

@nlutsenko
Copy link
Contributor

CLANG_ENABLE_OBJC_ARC=YES shouldn't be required... Maybe you didn't checkout the submodules? The pure difference comes from the fact that we want to test both 32 and 64 bit versions of code (that's why 2 destinations). It is not required for development though, since you can simply Test the framework target, which will run all the tests ;)

It passes as a PR on Travis - that's the important part. You can ignore codecov there, since those guys changed a bunch of things and it still is semi-stable.

NSError *error = task.error;
XCTAssertNil(error);
XCTAssertNotNil(task.result);

Copy link
Contributor

Choose a reason for hiding this comment

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

The checks for userId and screenName were still useful, sorry if I was not explicit enough about this portion. I feel like it's fine to leave it as is, but if you want to update it till next morning (will merge it then) - feel free to update this part.

@nlutsenko nlutsenko self-assigned this Apr 28, 2016
@nlutsenko nlutsenko added this to the 1.10.1 milestone Apr 28, 2016
@nlutsenko
Copy link
Contributor

Looks great. Will merge in tomorrow morning.

@zadr
Copy link
Contributor Author

zadr commented Apr 28, 2016

Re-added the asserts.

And, hm. There aren't any submodules in the projects, and, I had to run pod install to get OCMock. I'll assume I'm just missing something obvious 'cos of the time, and not worry about it, though!

To run tests in Xcode, (opening up ParseTwitterUtils.xcworkspace and hitting ⌘U), I had to make the same changes to the project to enable ARC and to set the Base SDK (not deployment target, as I said earlier, whoops) to Latest iOS SDK before they'd run.

Thanks again for reviewing this so quickly (and so late^wearly, this time :D)

@nlutsenko
Copy link
Contributor

We actually do have a submodule there in Vendor folder for xctoolchain which contains the common xcconfig files for all of the configurations (including enabling ARC).
Try running git submodule update --init --recursive in your local clone 😁

@zadr
Copy link
Contributor Author

zadr commented Apr 28, 2016

(The obvious thing i'm missing is that git submodule update doesn't do anything until git submodule update --init is run first 😓)

@nlutsenko
Copy link
Contributor

Perfect, merging now. Just sent a PR as well for updating all dependencies and Travis CI to Xcode 7.3, so we are going to get better on that front as well.

@nlutsenko nlutsenko merged commit 3378fa3 into parse-community:master Apr 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants