refactor(files) move serving files into a plugin #130
Conversation
Tieske
force-pushed
the
files
branch
2 times, most recently
from
3a3d433 to
7c6391c
Compare
Tieske
force-pushed
the
files
branch
2 times, most recently
from
853998e to
b758301
Compare
| server:start(function(req) | ||
| local data = req:post() | ||
| server:start(function(req, resp) | ||
| local stop = false |
There was a problem hiding this comment.
You don't need this variable, you can simply return the values.
There was a problem hiding this comment.
That's what I originally had. I introduced the stop variable for readability. It makes it clear what the returned result is supposed to be. If results are directly returned one has to mentally parse every return statement, whereas this reads easy as the intent is clear.
There was a problem hiding this comment.
let me know if you want me to change this anyway. No problem.
There was a problem hiding this comment.
I disagree that it's making it more readable, but I'm fine to keep it there as it's just a code example :)
|
|
||
| local path = req:path() | ||
| if req:method() ~= "POST" or path ~= "/index.html" then | ||
| return stop |
There was a problem hiding this comment.
| return stop | |
| return false |
| end | ||
| stop = not not resp:writeFile("./example/root" .. path) |
There was a problem hiding this comment.
| stop = not not resp:writeFile("./example/root" .. path) | |
| return not not resp:writeFile("./example/root" .. path) |
| end | ||
| stop = not not resp:writeFile("./example/root" .. path) | ||
| return stop |
|
I think we should add more unit tests here. We have entire new modules without any tests. |
Tieske
force-pushed
the
files
branch
4 times, most recently
from
ee4642c to
1ac5fdb
Compare
|
Finally had some time to write the tests for the 2 new files; test for the json lib, and the files-plugin. |
Tieske
force-pushed
the
files
branch
5 times, most recently
from
4f29a4a to
b13f5af
Compare
this reduces the 'handler' to primary task of running plugins
|
@EvandroLG I rebased the PR, afetr some of the commits were merged via other PR's. What's left is hard to simplify due to conflicts that would arise. Please have another look. |
|
@EvandroLG anything I can do to get this moving? |
|
Super nice stuff, @Tieske! Thanks! :) |
|
Added the requested docs. Accidentally also pushed the router plugin, but removed that commit again. The files plugin was already in the rockspec. So all changed as requested. |
| handler.plugins = plugins or {} | ||
|
|
||
| if location then | ||
| handler.plugins[#handler.plugins+1] = Files:new { |
There was a problem hiding this comment.
I'm unsure if I agree that a plugin should be called into the handler logic. In my view, the handler logic should work just with stuff that is part of the Pegasus core. I'm curious to see how you envision it.
src/pegasus/json.lua
Outdated
| @@ -0,0 +1,62 @@ | |||
| -- Wrapper library that supports multiple JSON libraries | |||
There was a problem hiding this comment.
It's more like a plugin, right? Why do you think it should be served as part of the Pegasus root?
src/pegasus/plugins/tls.lua
Outdated
| return false | ||
| end | ||
| print"running TLS" | ||
| --print"running TLS" |
There was a problem hiding this comment.
| --print"running TLS" |
| server:start(function(req) | ||
| local data = req:post() | ||
| server:start(function(req, resp) | ||
| local stop = false |
There was a problem hiding this comment.
I disagree that it's making it more readable, but I'm fine to keep it there as it's just a code example :)
|
Looks great, @Tieske! Thanks! |
This builds on top of #127
this reduces the 'handler' to primary task of running plugins