Allow specifying the error type for NewMiddleware and related traits

[msrd0]

I just tried to implement NewMiddleware for my own type and realised that I had an error that could occur but was not of type std::io::Error. Returning io::Error::Other along with a custom error message would work but I believe using the ? shorthand is much easier.

This is why I introduced NewMiddleware::Err type to be specified manually. For most implementations, std::convert::Infallible is the appropriate error type (this is at least true for all types that use the derive macro), and those that wish to continue returning io::Result can simply specify type Err = io::Error. So this change does not hurt existing implementations but makes future ones much more convenient.

[codecov]

Codecov Report

Merging #430 into master will increase coverage by 0.79%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   83.41%   84.21%   +0.79%     
==========================================
  Files         106      106              
  Lines        5156     5225      +69     
==========================================
+ Hits         4301     4400      +99     
+ Misses        855      825      -30     

Impacted Files	Coverage Δ
gotham/src/lib.rs	82.75% <ø> (ø)
gotham/src/middleware/logger/mod.rs	0.00% <0.00%> (ø)
gotham/src/middleware/security/mod.rs	0.00% <0.00%> (ø)
gotham/src/middleware/timer/mod.rs	0.00% <0.00%> (ø)
gotham_derive/src/new_middleware.rs	0.00% <ø> (ø)
middleware/template/src/lib.rs	0.00% <0.00%> (ø)
gotham/src/middleware/chain.rs	94.44% <100.00%> (-5.56%)	⬇️
gotham/src/middleware/cookie/mod.rs	80.00% <100.00%> (-6.67%)	⬇️
gotham/src/middleware/session/backend/memory.rs	89.89% <100.00%> (ø)
gotham/src/middleware/session/mod.rs	80.71% <100.00%> (+0.52%)	⬆️
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78a5e03...56b5ec1. Read the comment docs.

[nyarly]

I can definitely see the benefits to this. My only concern with merging ahead of 0.5 final would be the API churn involved. Maybe there aren't enough middleware in the wild to worry about this, and the update seems trivial, generally.

[msrd0]

Well, 0.5 can be a breaking change, so if you are happy with the API churn I don't see why this couldn't be merged. Unfortunately associated type defaults aren't stable yet, otherwise we could make this change a non-breaking one.

Also, the change to existing middlewares is rather minimal. Most people probably use the derive macro and therefore won't notice. Otherwise, if you don't care about the error type, you'd just add one line:

impl Middleware for FooMiddleware {
	type Error = io::Error; // the only new line
	// existing code
}

[pksunkara]

You can simply specify #[non_exhaustive] on this enum if we bump MSRV to 1.40

[pksunkara]

One thing I had recently tried to grapple with is that because of the associated types of NewMiddleware, we can't store the middlewares in an abstract form using trait objects. (which is why we needed the borrow_bag in the first place)

I understand that NewMiddleware returning another object as Middleware using Instance helps flexibility but I was wondering why they can't together be simply one trait without the Instance associated type.

To be honest, we don't need the Err associated type if we can specify an ECS type storage gotham::Error and use it for all middlewares like how tide does.

Basically, I am trying to make the usage of middlewares DRY. Currently, it isn't.

[msrd0]

Well, having an associated Err type is common practice in the std library, for example in the FromStr or TryInto traits.

That said, if gotham doesn't really do anything with the error, especially not returning it to the user, I don't see a reason for not using gotham::error::Error (and possibly switch this from failure, which has been deprecated, to anyhow, which is the suggested successor).

However, since @nyarly prefers to break less code, I guess it is easier for now to be able to still use std::io::Error, and switch to a generic error type in the future?

[pksunkara]

this-error is the suggested successor for libraries.

I don't understand why we don't want to do breaking changes. As long as they are documented, breaking stuff is always preferable because it will improve the library as a whole.

[msrd0]

anyhow is the replacement of the generic error type while thiserror only provides the derive macro.

I though about this a bit further and I think that it actually makes more sense to allign NewMiddleware and NewHandler (which as of today returns failure::Result). Since the user does not get to handle the error, it makes no sense to use any concrete error type (including std::io::Error). This is why I created #438, which obsoletes this PR.

[pksunkara]

Closing this PR in favor of #438