- 
                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 lets
| 
               | 
          ||
| // 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.