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

feat(router): Add chainable routes #288

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(router): Add chainable routes #288

wants to merge 1 commit into from

Conversation

mattbanister
Copy link

This doesn't work yet but I'm putting up the PR because I could use some help with it.

}

impl<'a, D: Sync + Send + 'static> ChainablePath<'a, D> for Nickel<D> {
fn route(&'static mut self, path: &str) -> ChainableRoute<'a, D> {
Copy link
Member

Choose a reason for hiding this comment

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

These should work with &'a mut self rather than &static ..

@mattbanister
Copy link
Author

@Ryman Thanks very much for the help. Seems to be working now. Please take a look again when you have a chance and let me know if you have any feedback. I'm still very much new to rust. So, any feedback is appreciated.

One question I had was whether Rust can distinguish between trait methods with the same name but different arity. When I try to import both my chainable router impls and the HttpRouter ones, I get a compilation error that the "get" method for example exists in both traits. So I'm thinking it can't tell that they have different signatures/arities. Maybe this is just a limitation of rust at the moment?

@cburgdorf
Copy link
Member

So I'm thinking it can't tell that they have different signatures/arities

Yep, there's no such thing as method overloading in Rust and it's probably unlikely to be implemented.

However, you can always just shift syntax to specifically tell the compiler which method to invoke.

So, instead of writing this which would be forbidden:

use TraitWithGet;
use OtherTraitWithGet;

some_thing.get();
some_thing.get(true); //compiler error

You could write:

use TraitWithGet;
use OtherTraitWithGet;

TraitWithGet::get(some_thing);
OtherTraitWithGet::get(some_thing, true);

I didn't test it out but I'm pretty sure it should work.

UPDATE

Corrected syntax according to @Ryman 's comment. Stupid me ;)

@Ryman
Copy link
Member

Ryman commented Oct 24, 2015

What @cburgdorf is referring to is UFCS but it should be SomeTrait::method(instance_of_self, other_args,..) rather than using . :)

You shouldn't really be running into the method name clash unless you've implemented both traits for a given type though? (has the code changed?) Inference should be able to see that the return from route has a get that is very different from the HttpRouter::get.

@mattbanister
Copy link
Author

@Ryman @cburgdorf

You shouldn't really be running into the method name clash unless you've implemented both traits for a given type though? (has the code changed?) Inference should be able to see that the return from route has a get that is very different from the HttpRouter::get.

Right, that's true. I confused myself. I had an intermediate implementation where I was just adding the last path added with route to the Nickel stuct (which is ugly anyway). In that case, all of the methods were implemented on Nickel rather than ChainableRoute. My question actually doesn't apply to this PR. Thanks for the help guys!

@mattbanister
Copy link
Author

@Ryman @cburgdorf Is there anything I can do to improve this? Any feedback is appreciated as I'm new with rust.

use router::http_router::{HttpRouter};

pub struct ChainableRoute<'a, D: Sync + Send + 'static = ()> {
path: String,
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be &'a str, or you might need to introduce a new lifetime 'b if the route chaining is to work (haven't tried)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I'm trying to hack around my limitations in understanding lifetimes. I'll try that and see if I can get it working with an &str.

@Ryman
Copy link
Member

Ryman commented Nov 4, 2015

@mattbanister Sorry for the delay, I've left some feedback :)

At a higher level though, do you think you might want to publish this as a crate? I think it builds nicely around the core so I think it would be a good example of how to make extensions to nickel if you wished to do so!

@mattbanister
Copy link
Author

Hi @Ryman

Thanks for the feedback. I think that would be great to make it a separate crate. I'll work on that and your other comments this week. I really appreciate the help!

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.

3 participants