-
Notifications
You must be signed in to change notification settings - Fork 282
A more Swift friendly route registration #136
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
Conversation
42184d7
to
ccc1d0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, consider updating it to use Swift 4.
💯 |
|
||
|
||
/** | ||
A Swift friendle version of -registerBlock:forRoute: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Friendly
README.md
Outdated
|
||
Swift | ||
````swift | ||
self.router.registerRoute("/log/:message") { link in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would ideally be changed to come through as register(route:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't because there's already a method called -registerBlock:forRoute:
which gets the shorthand register:
. Changing the order in the header would do it but also breaks the current API. Best we can do is define it as register:routeHandlerBlock:
// Register a block to a route (matches dpl:///product/93598) | ||
self.router.registerRoute("/product/:sku") { link in | ||
|
||
if let rootViewController = application.keyWindow?.rootViewController as? UINavigationController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should use guard let
and you might want to use optional chaining so you don’t need multiple let
s
|
||
// Register a class to a route using the explicit registration call | ||
self.router.registerRoute("/log/:message") { link in | ||
if let link = link { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the link passed to the block non-optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That's the plan but at the moment we don't have any nullability annotations and adding them technically breaks the API. I'd rather do this in 2.0.
The focus of this change is to disable subscript registration in Swift because the blocks were ignored internally and never called. It looks like you can't even use that API in Swift 4.
ccc1d0b
to
1987978
Compare
1987978
to
0a4d77a
Compare
The focus of this change is to disable subscript registration in Swift because the blocks were ignored internally and never called.