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

[IdentityLookup] Adds Xcode 9 Beta 1 bindings #2273

Merged
merged 1 commit into from
Jun 30, 2017

Conversation

dalexsoto
Copy link
Member

No description provided.

@monojenkins
Copy link
Collaborator

Build failure

@dalexsoto
Copy link
Member Author

@dalexsoto
Copy link
Member Author

build

[DisableDefaultCtor]
[BaseType (typeof (NSExtensionContext))]
interface ILMessageFilterExtensionContext {

Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace

Copy link
Member Author

Choose a reason for hiding this comment

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

I always leave a Whitespace between the interface line definition and the first member definition and this is the first time I've been called out :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not a big deal either way, and I don't think there's a policy for it. I never leave the whitespace there, and glancing through a couple of the larger binding files most interfaces seemed to not have it, though looking elsewhere there's plenty that DO have it, so...¯_(ツ)_/¯

[iOS (11,0)]
[Protocol]
interface ILMessageFilterQueryHandling {

Copy link
Contributor

Choose a reason for hiding this comment

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

Same (and others after)

Copy link
Member Author

Choose a reason for hiding this comment

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

I always leave a Whitespace between the interface line definition and the first member definition and this is the first time I've been called out :)

@dalexsoto
Copy link
Member Author

@timrisi see for example intents https://github.com/xamarin/xamarin-macios/blob/master/src/intents.cs do you have a reference to our coding guidelines where I shouldn't do so?

@timrisi
Copy link
Contributor

timrisi commented Jun 30, 2017

Don't know of any guidelines. Foundation mostly seems to not have it, so I this is probably one case with no current guideline

@monojenkins
Copy link
Collaborator

Build failure

@dalexsoto
Copy link
Member Author

unrelated failure

Copy link
Contributor

@spouliot spouliot left a comment

Choose a reason for hiding this comment

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

pending check for swift name


[Abstract]
[Export ("handleQueryRequest:context:completion:")]
void HandleQueryRequest (ILMessageFilterQueryRequest queryRequest, ILMessageFilterExtensionContext context, Action<ILMessageFilterQueryResponse> completion);
Copy link
Contributor

Choose a reason for hiding this comment

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

that's repeating QueryRequest from the first parameter
but Handle is not very descriptive and already used for a property
can you just double-check the swift name ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@spouliot they use handle in swift func handle(ILMessageFilterQueryRequest, context: ILMessageFilterExtensionContext, completion: (ILMessageFilterQueryResponse) -> Void) but Handle on our side has a very special meaning and also can conflict if implemented by a NSObject which will cause additional pain (code) on the users end. Also for readability HandleQueryRequest IMHO is much more descriptive.

I think we can bend our rules a little this time but it is your call 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I agree - let's merge that puppy.

@dalexsoto dalexsoto merged commit 56af068 into xamarin:xcode9 Jun 30, 2017
@dalexsoto dalexsoto deleted the identitylookupb1 branch June 30, 2017 02:22
mandel-macaque added a commit to mandel-macaque/xamarin-macios that referenced this pull request Aug 5, 2020
…rin#2273

Make the test check against the value returned by the native API rather
than expect the result to always be null.

Fixes xamarin/maccore#2273
rolfbjarne pushed a commit that referenced this pull request Aug 6, 2020
#9276)

* [Tests] Fix TryGetSupportedInterfaces when we have values. Fixes #2273

Make the test check against the value returned by the native API rather
than expect the result to always be null.

Fixes xamarin/maccore#2273

* Address reviews.
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.

5 participants