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

Add workers middleware and reengineer diesel middleware to make use of it #227

Closed
wants to merge 31 commits into from

Conversation

smangelsdorf
Copy link
Contributor

The best (read: laziest) way to describe this PR is by pointing at the documentation for the "workers" and "diesel" middleware as written for this PR (mostly the rustdoc).

Ref #210

@smangelsdorf
Copy link
Contributor Author

Ah, needs a rebase to work with Gotham master

@codecov
Copy link

codecov bot commented Jun 14, 2018

Codecov Report

Merging #227 into master will increase coverage by 3.21%.
The diff coverage is 78.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
+ Coverage    79.1%   82.31%   +3.21%     
==========================================
  Files          83       36      -47     
  Lines        3963     1674    -2289     
==========================================
- Hits         3135     1378    -1757     
+ Misses        828      296     -532
Impacted Files Coverage Δ
...dleware/under_development/diesel/src/state_data.rs 100% <100%> (+100%) ⬆️
gotham/src/middleware/workers/pool.rs 76.66% <76.66%> (ø)
gotham/src/router/response/extender.rs 13.63% <0%> (-11.37%) ⬇️
gotham/src/service/timing.rs 56.52% <0%> (-5.98%) ⬇️
gotham/src/state/request_id.rs 97.56% <0%> (-2.44%) ⬇️
gotham/src/router/tree/node.rs 90.28% <0%> (-1.26%) ⬇️
gotham/src/test/request.rs 92.85% <0%> (-0.9%) ⬇️
gotham/src/router/route/matcher/accept.rs 0% <0%> (ø) ⬆️
misc/borrow_bag/src/append.rs 100% <0%> (ø) ⬆️
gotham_derive/src/extenders.rs 0% <0%> (ø) ⬆️
... and 68 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 d31462a...872906c. Read the comment docs.

@whitfin
Copy link
Contributor

whitfin commented Jul 15, 2018

@smangelsdorf by any chance, do you want to finish this up? I think the workers middleware might be a good thing to ship alongside gotham itself, rather than being separated - we could maybe make use of it internally that way (and other people can work with it more easily).

@smangelsdorf
Copy link
Contributor Author

Sure, will do. I kept it separate while it was an experiment but I'm happy to move it into the main Gotham codebase.

@smangelsdorf
Copy link
Contributor Author

Done. Something's amiss with the clippy run, but otherwise all green.

@im-0
Copy link
Contributor

im-0 commented Jul 18, 2018

@smangelsdorf, this happens because of my patches to run Clippy from Travis. #257 should fix this.

@smangelsdorf
Copy link
Contributor Author

@im-0: Thanks, that sorted it 👍

@smangelsdorf
Copy link
Contributor Author

I still plan to sort out the reduced coverage and the new merge conflict, just haven't got to it yet.

@smangelsdorf
Copy link
Contributor Author

The changes here aren't compatible anymore with the master branch. The amount of work required to fix it probably warrants being submitted as a new PR (including weighing up whether the approach I took is still the correct one given the ecosystem as it exists now). I'm going to close this PR but leave the branch intact in case somebody wants to pick it up and continue working on it.

Recent releases of Rust may also allow some better refactoring, especially impl Trait which I note I've previously left a TODO comment about.

@whitfin
Copy link
Contributor

whitfin commented Aug 16, 2018

@smangelsdorf noted - I'll take a look and it and see if I can get your work into master, to avoid having you wasted your time

@smangelsdorf
Copy link
Contributor Author

The time is a sunk cost at this point, so don't worry about wasting it 😉

Hopefully some percentage of this is still useful in the new world of Tokio/Futures/Hyper, but if it's easier to throw it out and redo it differently I won't take it to heart.

@colinbankier
Copy link
Collaborator

@whitfin - I'm interested in seeing if I can resurrect this. I don't want to double up however, if you've already got far with it.

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.

None yet

4 participants