Not ready yet, but more progress

I saw that the dependencies broke today again. I updated them so we can keep working on this. Are you still working on this @rgs1? If not I might try to finish it during the weekend.

Ok so @rgs1 I have fixed most the errors from the latest nightly, but I have no idea what the following construct's are doing so I don't even know how to begin to fix them.

json_body_parser.rs:34:16: 34:30 error: type parameters may not appear here [E0085]
json_body_parser.rs:34         self::<JsonBodyParser>().and_then(|&: parsed| {

If you can point me in the direction of what that language construct even is I can have a pull request for you that should fix all the build errors.

You must have edited that line.

What it originally says is self.get::<JsonBodyParser>().and_then(|parsed|.

What that is supposed to do is the following:

• Retrieve the JsonBodyParser from the typemap.
• If it was found, execute the function on the following line.
  • If it wasn't found, return without doing anything.

I assume the confusion comes from the fact that you have edited the line.

I'm super glad to see new contributors by the way!

Ah very nice @dkhenry! I'll resume this today!

👍

Ok! It builds! It's not ready for merge because there are some inefficient things (i.e.: assigning a Regex::new to a static inside a mod is not working for me, need to dig in) plus in general things are not super tidy. I also haven't touched the tests.

I'll make a 2nd pass now to get things in order, unless someone gets there first.

(I updated the branch, but it's still WIP)

Ah, so regex! is available via a separate lib (plugin), regex_macros. So the static issue is fixed, next warnings + tests!

No more warnings! Only tests are left to be fixed!

Beautiful! I will have a look at the code later (as will the others, I'm sure. @Ryman always manage to find some nitpicks :) ). When you feel that you're done, would you mind naming your commit according to the following conventions? I forgot to tell you on your last commit but we're looking at automatically generating a changelog in the future.

Tests build!! They don't pass though :-) But I think I mostly know why. Going out for a run and will fix them when I get back. Almost there! Stay tuned

You have changed the mutability on the Request parameter to some handler functions in the examples where it isn't necessary. Interestingly enough, the compiler doesn't warn us that the mutability isn't needed. Anyways, as we don't need it to be mutable in these examples we might just as well change that back.

And what's up with storing the handler in a variable like that? Are you sure issue #15444 is related? Because this was working a month and a half ago and that issue is older, no?

Oops, sorry about the unneeded mut. Sure I'll fix all the unneeded stuff on a final pass (making tests pass now).

Wrt to coercing handlers, I couldn't find another way to make it work with nightly. Here's a toy example: http://is.gd/H60Xjl.

I asked around #rust, and they say it'll get fix at some point.

No worries. Sounds good!

Yeah, I also figured that it must be a bit of a bug in the compiler. I questioned whether issue #15444 that you mentioned in a comment is the one to blame, as that issue dates from before this stopped working. Anyways, we have the router! macro available so people won't have to use that workaround if the don't want to :)

We'll have to remember to revert the examples once this gets fixed in the compiler just.

Yup I'll try to find the actual real/current issue in so we can mention that in the FIXMEs

All tests passing! Now, we can do some real review @simonpersson ;-)

Up next:

• drop the unneeded &mut
• format the commit according to the contributing guidelines

Please feel free to point out nits (and not nits) so we can get this in good shape. Thanks!

Updated the commit message according to the used convention + drop the unneeded &mut changes. Should be ready-ish for merge hopefully.

Yay! Travis passed too, great news.

The not_found_handler in the file nickel.rs is commented out (line 142). Is there a reason for that or did you just miss it?

Some calls to unwrap() are introduced. These are not good, as they can cause the server to panic. For example, line 108 in response.rs was changed from using and_then to unwrap. Can't we use and_then for some reason here? If we can't use and_then for some reason, I suppose we can use normal pattern matching. Could you look at all the calls to unwrap() that you introduced and see if they can be eliminated (preferably with an error message on the failing case where it makes sense, like in the creation of regexes from handler path strings)? I think it's preferable that the server is misbehaving on incorrect data rather than crashing.

Line 140 in response.rs was moved, now the comments there doesn't make much sense. Line 139 and 140 should be switched.

Other than these things, I think this looks great! I'd like to hear the opinion of @Ryman or @cburgdorf before merging though,

Great, thanks for the review! I'll take a look in a bit, all of those are probably left over or omissions.

Wrt to the rust-http issue, in nickel-org/rust-http@53d30c0 I wonder if:

-impl fmt::Show for Status {
+impl fmt::Display for Status {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
    write!(f, \"{} {}\", self.code(), self.reason())
}

has something to do.

Added a few inline comments, looks good other than that, and we can iterate easier once master's buildable :) I've let an old branch porting to hyper bitrot, might try to rebase onto this sometime soon.

Thanks for the efforts @rgs1!

@Ryman - Hyper sounds cool (and needed)!

@rgs1 - I pushed a fix to the http repo, reverting your changes in that file. Now it works again :)

@simonpersson: thanks! Though Show is deprecated and was split into Debug and Display. I think we want both for status (i.e.: impl Display and derive Debug). We can follow-up in that repo.

@Ryman: thanks for the review! I'll get to that a bit later, would be great to have master be buildable again!

Updated to conform with @Ryman's review - let me know how it looks now.

@rgs1 Thanks, I think I checked all the changes, still a few outstanding nits.

In future, could you make changes in new separate commits that can be squashed later, as it helps to minimize incremental review time. 👍

Added two commits to address the last two comments. Shall we wait for @simonpersson to weight in on how to handle the casts?

Note that the last two commits don't adhere to the usual commit formatting, since we'll probably squash them.

Thanks for the review @Ryman.

LGTM once there's a consensus on the casting/ergonomics issue :)

Have we tried declaring a type for easier casting? If it works it is prettier I suppose. On the other hand, it's not meant to be a permanent solution anyways and there's still the router!-macro for those who value ergonomics.

So, it doesn't really matter to me :)

I assume we'll be tracking nightly from now on, so things will be in a state of flux anyway. As soon as this is fixed in the compiler, we can change it.

Exactly. I say we merge this as it is.

@rgs1 - Can you squash the commits to make it ready for merging?
@Ryman - You press the button if you can't think of anything more!

Squashed @simonpersson @Ryman !

Awesome work @rgs1 :)

Merged, thanks for the work @rgs1! 👍