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

[WIP] Compatibility with Swift 3.0 (up to Xcode 8 GM) #267

Closed
wants to merge 19 commits into from
Closed

[WIP] Compatibility with Swift 3.0 (up to Xcode 8 GM) #267

wants to merge 19 commits into from

Conversation

nighthawk
Copy link
Contributor

@nighthawk nighthawk commented Aug 17, 2016

This includes both changes suggested by Xcode's migration tool as well as manual fixing of the issues that Xcode couldn't migrate.

Action list:

  • Double-check Int+OAuthSwift.swift
  • Code review
  • Squash commits
  • Get tests to work again
  • Fix test failures

@cambuilt
Copy link

I needed to make additional manual migration changes for Xcode 8, beta 6, but when trying to authenticate to Yelp, I get INVALID_SIGNATURE consistently and despite numerous code reviews. Using the Swift 2.0 version of OAuthSwift with Xcode 7.3 however, it works fine.

Any ideas?

@nighthawk
Copy link
Contributor Author

@dongri, @rvangraan, can either of you double check Int+OAuthSwift.swift. The migrator failed on that file and I'm not sure if I manual migration is correct.

@nighthawk
Copy link
Contributor Author

I'm still stuck on getting the tests to work again as Erik and its dependencies haven't yet been upgraded to Swift 3. @phimage, any advice on that?

@cambuilt
Copy link

I have given up on OAuthSwift for Swift 3 and using simple URLRequest and URLSession with dataTasks to authenticate to Yelp. I was only using maybe 1% of the capability of OAuthSwift and was getting tired of fixing the code for every new beta of Xcode 8.

Sent from my iPad Pro

On Aug 26, 2016, at 9:57 PM, Adrian Schoenig notifications@github.com wrote:

I'm still stuck on getting the tests to work again as Erik and its dependencies haven't yet been upgraded to Swift 3. @phimage, any advice on that?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@phimage
Copy link
Member

phimage commented Aug 27, 2016

@cambuilt swift3 is in beta, xcode is in beta, what do you expect...
When coding in Swift 3 beta, get accustomed to updating your previous code
You can make a local repo or remote one with your own updates... (a pod update will not erase your change)

@nighthawk I make a swift3.0 branch today. There is some bug in unit test but all compile
Thanks for all your work

@nighthawk
Copy link
Contributor Author

Tests are compiling again and all but OAuth2SwiftTests.testExpire() are succeeding.

Small adjustment to OAuthSwiftHTTPRequest.start() to pass on HTTP response error code rather than using default value of -1
@@ -160,17 +163,16 @@ open class OAuthSwiftHTTPRequest: NSObject, URLSessionDelegate, OAuthSwiftReques
}
}
} else {
localizedDescription = OAuthSwiftHTTPRequest.descriptionForHTTPStatus(self.response.statusCode, responseString: String(data: self.responseData as Data, encoding: self.dataEncoding)!)
errorCode = httpResponse.statusCode
Copy link
Contributor Author

@nighthawk nighthawk Aug 28, 2016

Choose a reason for hiding this comment

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

This line fixes testExpire as otherwise -1 would be returned.

(Don't get confused by GitHub showing that localizedDescription got removed, it just moved down one line.)

Copy link
Member

Choose a reason for hiding this comment

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

Yes that was fixed in master branch

@nighthawk
Copy link
Contributor Author

All tests are succeeding now.

@cambuilt
Copy link

I thought a fork would at least build, but let me know if that should not be expected, since I am relatively new to Github.

On Aug 27, 2016, at 1:09 PM, Eric Marchand notifications@github.com wrote:

@cambuilt https://github.com/cambuilt swift3 is in beta, xcode is in beta, what do you expect...

When coding in Swift 3 beta, get accustomed to updating your previous code
You can make a local repo or remote one with your own updates... (a pod update will not erase your change)

@nighthawk https://github.com/nighthawk I make a swift3.0 branch today. There is some bug in unit test but all compile
Thanks for all your work


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #267 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ALClF3zS67XavXLY1NehB3UeXloF1KQ5ks5qkG9XgaJpZM4JmIPd.

@nighthawk
Copy link
Contributor Author

nighthawk commented Aug 28, 2016

@cambuilt, this pull request is marked as WIP, which stands for Work in Progress.

For you issue, it would be much appreciated if you can provide a test case to SignTests.swift with the inputs and the expected signature.

@cambuilt
Copy link

Not sure how I can do that without giving you my client id and secret, in this case.

On Aug 28, 2016, at 6:40 PM, Adrian Schoenig notifications@github.com wrote:

@cambuilt https://github.com/cambuilt, this pull request is marked as WIP, which stands for Work in Progress.

For you issue, it would be much appreciated if you can provide a test case to SignTests.swift with the inputs and the expected signature.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #267 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ALClF25uh_eIMuqpLao7FGuGMKwfTBxQks5qkg5FgaJpZM4JmIPd.

@nighthawk
Copy link
Contributor Author

You could replace those with random strings and see if the different versions of OAuthSwift still come up with different signatures.

From what I've noticed in the past (#165 and #168), the issues were with other parts of the input than the client ID and secret.

@cambuilt
Copy link

Wouldn't the oauth_timestamp have to be identical for both to get a valid result?

On Aug 28, 2016, at 6:58 PM, Adrian Schoenig notifications@github.com wrote:

You could replace those with random strings and see if the different versions of OAuthSwift still come up with different signatures.

From what I've noticed in the past (#165 #165 and #168 #168), the issues were with other parts of the input than the client ID and secret.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #267 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ALClFzrUI9s2TYbLIAZ8pPZ0P4F4E1Maks5qkhKsgaJpZM4JmIPd.

@nighthawk
Copy link
Contributor Author

A fixed timestamp is fine for the tests - check out the other tests for comparison.

@cambuilt
Copy link

Thanks.

On Aug 28, 2016, at 7:10 PM, Adrian Schoenig notifications@github.com wrote:

A fixed timestamp is fine for the tests - check out the other tests for comparison.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #267 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ALClF0iHQJwzE5jJqL-aEO6j9eUnCszBks5qkhV6gaJpZM4JmIPd.

@nighthawk nighthawk changed the title [WIP] Compatibility with Swift 3.0 (up to Xcode 8 beta 6) [WIP] Compatibility with Swift 3.0 (up to Xcode 8 GM) Sep 8, 2016
@phimage
Copy link
Member

phimage commented Sep 11, 2016

I think some function renaming is needed to follow the new guidelines by omitting needless words.
but can be done after merging into master if conflict appear when doing it
examples:

  • authorizeWithCallbackURL(_ callbackURL: URL.. -> authorize(callbackURL: URL
  • postOAuthAccessTokenWithRequestTokenByCode(_ code: String.. ->
  • renewAccessTokenWithRefreshToken(_ refreshToken: String ->

@s-aska
Copy link
Contributor

s-aska commented Sep 13, 2016

ci you apply this fix has been successful.
https://travis-ci.org/s-aska/TwitterAPI/builds/159495700

github "skedgo/OAuthSwift" "63bdf693004537f23436c8330cd9410e5ad88a54"

Very Thanks.

This fixes When you are merge.

But probably OAuthSwiftTVOS and OAuthSwiftWatchOS requires a convert.

OAuthSwiftTVOS

“Use Legacy Swift Language Version” (SWIFT_VERSION) is required to be configured correctly for targets which use Swift. Use the [Edit > Convert > To Current Swift Syntax…] menu to choose a Swift version or use the Build Settings editor to configure the build setting directly.
warning: no umbrella header found for target 'OAuthSwiftTVOS', module map will not be generated

OAuthSwiftWatchOS

“Use Legacy Swift Language Version” (SWIFT_VERSION) is required to be configured correctly for targets which use Swift. Use the [Edit > Convert > To Current Swift Syntax…] menu to choose a Swift version or use the Build Settings editor to configure the build setting directly.
warning: no umbrella header found for target 'OAuthSwiftWatchOS', module map will not be generated

@phimage
Copy link
Member

phimage commented Sep 13, 2016

Not a real convert, just open the targets > build settings look for “Use Legacy Swift Language Version”. Changer "Unspecified" to "No"

@phimage
Copy link
Member

phimage commented Sep 15, 2016

old comment

  • open the targets > build settings look for “Use Legacy Swift Language Version”. Changer "Unspecified" to "No"
  • rename some functions

@max-potapov
Copy link

@phimage OAuthSwift:swift3.0 is 17 (!) commits behind the OAuthSwift:master and missing some parts of latest release like OAuthWebViewControllerDelegate for example.

May I ask you to rebaseswift3.0 branch and this PR to master branch? It will allow to use OAuthSwift with applications which was already converted to Swift 3.0 syntax.

@phimage
Copy link
Member

phimage commented Sep 22, 2016

If @nighthawk can squash its commits until 3 days I will merge into master branch ( and do some change function naming, etc...).
If not I will open master branch codes into xcode 8 and do the conversion job (but I want to keep @nighthawk work if possible, but not 19 commits)

@nighthawk nighthawk mentioned this pull request Sep 23, 2016
5 tasks
@nighthawk
Copy link
Contributor Author

nighthawk commented Sep 23, 2016

@phimage, I've squashed the commits and opened a new PR: #277. It's still based on the previous swift 3.0 branch as I'm not entirely comfortable with merging in the changes that have since been made to master (due to the merge conflicts).

@nighthawk nighthawk closed this Sep 23, 2016
@nighthawk nighthawk deleted the swift3.0 branch September 23, 2016 05:16
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