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.

Sign up for GitHub

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

Jump to bottom

RSA9h #491

Merged
merged 9 commits into from
Sep 29, 2016
Merged

RSA9h #491

merged 9 commits into from
Sep 29, 2016

Conversation

EvgenyKarkan
Copy link
Contributor

@EvgenyKarkan EvgenyKarkan commented Sep 4, 2016 •
edited
Loading

7c266e7

@EvgenyKarkan EvgenyKarkan changed the base branch from master to RSA8e September 4, 2016 17:25
mattheworiordan
mattheworiordan reviewed Sep 7, 2016
View reviewed changes
Spec/Auth.swift
tokenParams.capability = "{ \"test:*\":[\"test\"] }"

waitUntil(timeout: testTimeout) { done in
rest.auth.createTokenRequest(tokenParams, options: authOptions) { tokenRequest, error in
Copy link
Member

Choose a reason for hiding this comment

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

This API does not look right. Why is this options: when the IDL specifies we have two args, TokenParams and AuthOptions. Whilst I appreciate this PR is not necessarily about this, it's important we fix things as we go along. Can you take a look please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This API does not look right

Why do you think so? createTokenRequest API has 3 arguments - ARTTokenParams and ARTAuthOptions objects and a callback which provides ARTTokenRequest and an error if any.

Why is this options:

Lib written in ObjC, and tests in Swift, so options: is a part of ObjC method signature here and the argument is authOptions object of ARTAuthOptions class.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this might be an "iOS" thing then that I have misunderstood. I have never written iOS code so just looking at these PRS with a view to a) does it achieve the spec, b) does it follow the API. @tcard can you comment on this and confirm you're happy to leave as is i.e. I was wrong :)

Thanks @EvgenyKarkan for the explanation

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes @mattheworiordan, we're forced to do this every time we have more than one argument. It's just how Objective-C works. (When the last argument is a callback, as a syntactic convenience Swift lets you omit its name and pass it kind of like a Ruby block, that's why you don't see its name.)

@mattheworiordan mattheworiordan mentioned this pull request Sep 7, 2016
Merged
@EvgenyKarkan
Copy link
Contributor Author

Please review and merge if everything looks good.

@tcard
Copy link
Contributor

tcard commented Sep 13, 2016

Nice, but there's still one case missing: the case in which defaultOptions has field non-nil, but then options has that same field as nil. If we were merging the options as we were doing before, the non-nil (ie. the old) version would prevail, but because we're not merging anymore, it should behave as if the field were nil.

For example, you could set defaultOptions.authCallback to something, options.authCallback to nil and check that you get some error like "I have no means to authorize" and that the defaultOptions.authCallback wasn't called.

@EvgenyKarkan
Copy link
Contributor Author

Please review and merge if everything looks good.

@ricardopereira
Copy link
Contributor

ricardopereira commented Sep 28, 2016 •
edited
Loading

@EvgenyKarkan What's the state on this one? This PR will merge into branch:RSA8e but that branch has been already merged with master. Can I close this and create a new PR pointing to master? (UPDATE: oh, I can change the base branch of this PR. Nice 🙃. Will rebase with master.)

@ricardopereira ricardopereira changed the base branch from RSA8e to master September 28, 2016 10:23
@EvgenyKarkan
Copy link
Contributor Author

Hello @ricardopereira

What's the state on this one?

Hope it's done, I asked Toni for review this PR.

@ricardopereira
Copy link
Contributor

LGTM. I rebased with master but tests are failing.
I will look what's missing.

ricardopereira
ricardopereira reviewed Sep 29, 2016
View reviewed changes
Spec/Auth.swift
}

waitUntil(timeout: testTimeout) { done in
rest.auth.authorise(nil, options: options) { tokenDetails, error in
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we testing the createTokenRequest (spec (RSA9) Auth#createTokenRequest)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind. The createTokenRequest is being tested.

ricardopereira
ricardopereira reviewed Sep 29, 2016
View reviewed changes
Spec/Auth.swift
expect(currentTokenRequest).toEventuallyNot(beNil(), timeout: testTimeout)

waitUntil(timeout: testTimeout) { done in
rest.auth.authorise(nil, options: nil) { tokenDetails, error in
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, shouldn't we only testing the createTokenRequest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind. The createTokenRequest is being tested.

@ricardopereira
Copy link
Contributor

Nice, but there's still one case missing: the case in which defaultOptions has field non-nil, but then options has that same field as nil. If we were merging the options as we were doing before, the non-nil (ie. the old) version would prevail, but because we're not merging anymore, it should behave as if the field were nil.

For example, you could set defaultOptions.authCallback to something, options.authCallback to nil and check that you get some error like "I have no means to authorize" and that the defaultOptions.authCallback wasn't called.

@tcard Are you happy with this 8e63e9c?

@ricardopereira
Copy link
Contributor

All other PR's are related with this one, so I will merge it to help me out with my further work.
If anything needs to be adjusted just comment and I will address them into another PR.

@ricardopereira ricardopereira merged commit 31940a3 into master Sep 29, 2016
@ricardopereira ricardopereira deleted the RSA9h branch September 29, 2016 09:52
@tcard
Copy link
Contributor

tcard commented Sep 29, 2016

@ricardopereira I'd be happier if we were checking for a specific error code, since now any failure would make the test pass. But no biggie.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ricardopereira ricardopereira ricardopereira left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@EvgenyKarkan @tcard @ricardopereira @mattheworiordan