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

Added objc compatible post func #60

Merged
merged 2 commits into from
Nov 16, 2016
Merged

Added objc compatible post func #60

merged 2 commits into from
Nov 16, 2016

Conversation

ryang1428
Copy link
Contributor

  • The newer convenience functions for posting a notification are not compatible with objective-c. Using the old method throws warnings in our projects, which I would like to clean up
  • Added a simple post function that does not have any default parameters and uses the main queue by default.

@@ -17,6 +17,13 @@ public extension NotificationCenter {
}
}

// Needed for objc compatibility
public func post(notification: Notification) -> Void {
DispatchQueue.main.async { [weak self] in
Copy link
Contributor

Choose a reason for hiding this comment

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

this should just call through to the internal method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you want to include a Bool parameter similar to the old method to specify main thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah now calling the main func. As for the bool, we always are calling on the main thread so that isnt needed for FPs sake

@rmigneco
Copy link
Contributor

I'm Ok with the change but it seems like we lose some flexibility without being able to pass in a string name with optional object and userInfo properties.

Other possibilities:

  • overriding this to accept NSOperationQueue and re-implementing the methods which is pretty heavy
  • making the queue argument nullable for all but the root method. This will require providing the argument label even when the argument is nil in swift, which is annoying

oing to ask retail and ANT teams for feedback @codecaffeine @jgrandelli

@ryang1428
Copy link
Contributor Author

ryang1428 commented Nov 15, 2016

Note this is also merely a bandaid for objc. I can live with the warnings for the deprecated functions in FP until we move that all to swift. That is another option and we just close this PR. But We have 10 or so warnings in FP currently

@codecaffeine
Copy link
Contributor

@ryang1428 what warnings are you getting? Is it because functions with default parameters aren’t visible in Objective-C?

@ryang1428
Copy link
Contributor Author

Warnings are for using the deprecated functions. The new ones are not usable in objc as we discussed

@ryang1428 ryang1428 assigned jgrandelli and unassigned rmigneco Nov 16, 2016
@jgrandelli jgrandelli merged commit 8f1f56b into master Nov 16, 2016
@jgrandelli jgrandelli deleted the NotificationObjc branch November 16, 2016 18:15
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.

4 participants