Skip to content

Conversation

wessmith
Copy link
Member

@wessmith wessmith commented Sep 8, 2017

The focus of this change is to disable subscript registration in Swift because the blocks were ignored internally and never called.

@wessmith wessmith requested a review from pavelpantus September 8, 2017 16:34
@wessmith wessmith force-pushed the wes/introduce_swiftier_registration branch from 42184d7 to ccc1d0b Compare September 8, 2017 16:42
Copy link
Contributor

@pavelpantus pavelpantus left a 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.

@dasmer
Copy link
Contributor

dasmer commented Oct 14, 2017

💯



/**
A Swift friendle version of -registerBlock:forRoute:
Copy link
Contributor

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
Copy link
Contributor

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:)

Copy link
Member Author

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Member Author

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.

@wessmith wessmith force-pushed the wes/introduce_swiftier_registration branch from ccc1d0b to 1987978 Compare October 15, 2017 14:59
@wessmith wessmith force-pushed the wes/introduce_swiftier_registration branch from 1987978 to 0a4d77a Compare October 15, 2017 15:00
@wessmith wessmith merged commit 1f04de6 into master Oct 18, 2017
@wessmith wessmith deleted the wes/introduce_swiftier_registration branch October 18, 2017 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants