Exclusively path based routes - closes #20 #28
Conversation
Routes are now only distinct based on path components. The expected message and return types are only used to provide detailed error messages.
|
@amomchilov here's my first attempt at creating a PR instead of directly committing to the repo. Beyond giving me feedback on the actual code, please let me know if I've done anything wrong or suboptimal in how I've configured this PR. |
Adds a couple missing spaces
There was a problem hiding this comment.
Looks good, it's quite similar to what I had in mind.
I would suggest running this against the integration test I wrote (this is what I wrote it for, lol)
please let me know if I've done anything wrong or suboptimal in how I've configured this PR.
It's pretty good.
-
I would remove "addresses issue Type errors in routes are conflated with missing routes #20" from the title, and instead include
closes #20(or one of the other keywords that Github recognizes, likefixes,resolvesand their variants).When you do that, the pull request would automatically be listed on the issue's "Linked pull requests" list on the right (you can see a sample on Add real XPC integration tests #22).
It would also setup the reciprocal link, of listing the issue in this PR's "Linked issues" list
-
Rather than mentioning me with @amomchilov, you can directly tag me as a reviewer on the pull request. I haven't been doing that only because I don't have access to change issue/PR metadata (like links, assignees, reviewers and tags).
| } | ||
|
|
||
| /// Registers a route that has no message and expects a reply. | ||
| /// | ||
| /// If this route has already been registered, calling this function will overwrite the existing registration. Routes are unique based on their paths and types. | ||
| /// If this route has already been registered, calling this function will overwrite the existing registration. Routes are unique based on their paths. |
There was a problem hiding this comment.
Perhaps erroring would make more sense. Replacing one route with another of the same name would be quite wonky.
There was a problem hiding this comment.
Good point, it's hard to imagine a valid case for why an API user would intentionally want to do that.
There was a problem hiding this comment.
I've addressed this, although the solution I have isn't thread safe.
| let handler: () throws -> Void | ||
|
|
||
| func handle() throws { | ||
| func handle(request: Request, reply: inout xpc_object_t?) throws { |
There was a problem hiding this comment.
The approach you took here causes some duplication, but it has an opportunity to make more specific error messages.
If you're curious to compare notes, checkout what I came up with here: 96d9c92
There was a problem hiding this comment.
Ah I didn't realize you were working on this, sorry if I've caused wasted effort.
Looking at your approach, they're certainly similar although I prefer the approach I took a bit as it generates more detailed error messages and doesn't have any forced casts.
There was a problem hiding this comment.
Ah I didn't realize you were working on this, sorry if I've caused wasted effort.
That's kind of on me, I should have communicated that. In general, I have quite a bit more goodies built on top of this framework in my private repo, but it's generally a bunch of hacks and not-yet-shippable code.
I'm working to upstream them as fast as I can haha.
I prefer the approach I took a bit as it generates more detailed error messages
Indeed, I agree this is nicer. I was apprehensive to do it just because I was too lazy to code out all the cases :D
One critique I'll make however is that this has a "one error at a time" quality to it, which can be frustrating as a user. You run your code, get an error, fix it, run it again, get another error, and so on. It would be nice to have the full error (either your message type is wrong, or your reply type, or perhaps both) in one shot.
doesn't have any forced casts.
If you're new to Swift (though I doubt it, because you write pretty damn good code), I'd caution against always riding the "try!/!/as! is bad" train to its terminal station. I understand the appeal, but there's times (like in my sample, imo) where I think it can be appropriate to take these escape hatches.
The force casts simply assert "I can't express this type constraint in Swift's type system", but it doesn't mean "I didn't type-check this", because there's type checking of a different kind. I literally called my function "typecheck" 😄
There was a problem hiding this comment.
One critique I'll make however is that this has a "one error at a time" quality to it, which can be frustrating as a user. You run your code, get an error, fix it, run it again, get another error, and so on. It would be nice to have the full error (either your message type is wrong, or your reply type, or perhaps both) in one shot.
I'll centralize the error generation and then aggregate the error messages so they can be appended to one another.
I'd caution against always riding the "try!/!/as! is bad" train to its terminal station.
I agree with this sentiment, we're likely just at different points along the track. I find it valuable to within reason maximize the amount of issues the compiler can catch for me, so I try to avoid as! and ! in many circumstances for that reason.
My views on try! are quite different. I'm working on a service that needs very high reliability with structured error reporting back to a server in case of failure, so to simply crash the process when something goes wrong would be counterproductive - that's likely influencing the approach I'm taking here.
There was a problem hiding this comment.
I see, that makes perfect sense!
jakaplan
changed the title
Routes are now only distinct based on path components. The expected message and return types are only used to provide detailed error messages.