Eternity-Yarr commented 8 years ago

we select which content-type will be sent to the user based on request path and jsonp request parameter

Ogeon commented 8 years ago

I'll take an other look at it tomorrow, when I'm not this tired, but it looks good.

Ogeon commented 8 years ago

I'll also fix the error from AppVeyor... Again.

Eternity-Yarr commented 8 years ago

Looks like it's not AppVeyor fault, but slproweb simply replaced 1_0_2d with 1_0_2e version of openSSL :

404 https://slproweb.com/download/Win32OpenSSL-1_0_2d.exe 200 https://slproweb.com/download/Win32OpenSSL-1_0_2e.exe

Ogeon commented 8 years ago

Yeah, that's what I meant. There's unfortunately no way to just get the latest version...

Ogeon commented 8 years ago

Looks like rand is also broken now. I'll put this on hold and see how things plays out.

Update: Looks like a type inference regression: rust-lang/rust#30713

Eternity-Yarr commented 8 years ago

Seems like it s a new nightly :) not the same my build was failed on.

Eternity-Yarr commented 8 years ago

@homu retry ? (I just wanted to see if they fixed rand in current nightly, but @homu doesn't listen to me :) )

Ogeon commented 8 years ago

Nah, @homu will only listen to me at the moment :), but I restarted the tests manually. I haven't had the time to fix things yet...

Ogeon commented 8 years ago

90 contains the necessary changes. It looks like `rand` has been updated to work with nightly, so that's good, but I had to do a very fundamental change to `insert_routes!` to avoid the type ascription conflict.

Edit: Turns out that the problem was a different one, so the fix won't break anything after all.

I would also like to point you to #89 and ask you if you agree to dual license this PR and any future contributions to Rustful under MIT/Apache, as described there. I don't know when (or if) the relicensing will happen, so there is a chance that you will count as a "creative contributor" before it's done.

Ogeon commented 8 years ago

There we go. It's merged now, so you can rebase this PR and we can continue as before :smile:

Eternity-Yarr commented 8 years ago

Sure, i do agree with dual license, but i'm not sure if my pr is somewhat creative.

Ogeon commented 8 years ago

Alright :smile: It's not only for this, but for any potential future contributions as well.

Anyway, I see that you have rebased and the tests looks good, but what about the jsonp variant? I think the correct content type for it is application/javascript.

Eternity-Yarr commented 8 years ago

Sure, i'll add special case for jsonp. But this will require some refactoring of this particular test

Ogeon commented 8 years ago

Sure, go ahead.

Eternity-Yarr commented 8 years ago

(not to mention http://localhost:8080/json/Peter%22 case). But for now, I think working around this kind of issues would be an overkill for little example.

Ogeon commented 8 years ago

Hmm, yeah, its primary feature is to showcase filters. It doesn't have to be 100% correct in every case, but I just thought that adding a content type for one case and not the rest would be stranger than keeping it as it is...

Eternity-Yarr commented 8 years ago

Yeah, let's hope that no one would actually use it as a reference for his own json rest service :))

Ogeon commented 8 years ago

Haha, yeah. It's not great, but it's also hard to make simple examples of complex things. You are very welcome to play around with it if you happen to have any more ideas for improvements. That goes for every example here, really.

Eternity-Yarr commented 8 years ago

Sure, I'll try to do something tomorrow.

Eternity-Yarr commented 8 years ago

I've tried to do it as less intrusive as possible, because I'm new to the Rust.

Ogeon commented 8 years ago

Oh, interesting approach. My first impression is that I like it. :smile: It's once again late over here, so I will wait with the decision until tomorrow.

Also, don't worry about being new to Rust. I'll do my best to be a good mentor.

Ogeon commented 8 years ago

I have read through it now and I think it's good. I think this change is small enough to be squashed into one single commit, since the two solutions are quite different.

Would you also mind summarizing the changes in the top comment? It's what will go into the final commit message and will help anyone looking back on this to understand what happened. It doesn't have to be fancy. Just to answer the "how?" question.

Eternity-Yarr commented 8 years ago

You mean 'edit top comment of this pr' ?

Ogeon commented 8 years ago

Yes, since that's what people will read first. It's just so they don't have to wrap their heads around the code. This will do :smile:

Thanks!

@homu r+

Ogeon commented 8 years ago

Hmm... is @homu asleep?

Ogeon commented 8 years ago

Looks like it. I'll merge this myself, then. It's ok if it doesn't make it into the change log.

Ogeon / rustful

Setting appropriate content-type in filters example #87

90 contains the necessary changes. It looks like `rand` has been updated to work with nightly, so that's good, but I had to do a very fundamental change to `insert_routes!` to avoid the type ascription conflict.

Ogeon / rustful

Setting appropriate content-type in filters example #87

90 contains the necessary changes. It looks like rand has been updated to work with nightly, so that's good, but I had to do a very fundamental change to insert_routes! to avoid the type ascription conflict.

90 contains the necessary changes. It looks like `rand` has been updated to work with nightly, so that's good, but I had to do a very fundamental change to `insert_routes!` to avoid the type ascription conflict.