Closed CrowdHailer closed 5 years ago
I don't understand how the warnings that fail the CI come about...
It seems like it's caused by warnings_as_errors: true
, but I think the warnings only get emitted when running the tests:
➜ raxx git:(router) mix test
Compiling 20 files (.ex)
Generated raxx app
...warning: Routes should not be passed as arguments to `use Raxx.Router`.
Instead make use of the `section/2` macro.
See documentation in `Raxx.Router` for details
lib/raxx/router.ex:53
warning: The module `Raxx.RouterTest.OriginalRouter` already included the behaviour `Raxx.Server`.
This is probably use to `use Raxx.Server`,
this is no longer necessary when implementing a router.
test/raxx/router_test.exs:67
Compilation failed due to warnings while using the --warnings-as-errors option
➜ raxx git:(router) mix test
...warning: Routes should not be passed as arguments to `use Raxx.Router`.
Instead make use of the `section/2` macro.
See documentation in `Raxx.Router` for details
lib/raxx/router.ex:53
warning: The module `Raxx.RouterTest.OriginalRouter` already included the behaviour `Raxx.Server`.
This is probably use to `use Raxx.Server`,
this is no longer necessary when implementing a router.
test/raxx/router_test.exs:67
..................................................................................................................................................*.............................................................**...
Finished in 2.8 seconds
133 doctests, 83 tests, 0 failures, 3 skipped
Randomized with seed 624187
➜ raxx git:(router) mix compile
Compiling 20 files (.ex)
Generated raxx app
➜ raxx git:(router) mix clean && MIX_ENV=test mix compile
Compiling 20 files (.ex)
Generated raxx app
➜ raxx git:(router)
As you can see above, when you just compile or run the tests for the second time, it's not a problem.
I added some extra comments the the PR description. I think most of the issues I raised can be tackled later, i.e. lets experiment without implementations of logger and static before pushing them back to the Raxx repo.
That compiler warning is annoying, unless you can think of any smart fix I will skip the tests for the old interface. There is not much risk of regression as we should never hit that code
Should close https://github.com/CrowdHailer/raxx/issues/148
I wrote some code to fix the deprecation warning problem, merge it into this branch if you like it: https://github.com/CrowdHailer/raxx/pull/153
It's not perfect, but it's the most elegant robust solution I could think of. Hopefully we don't have to resort to this sort of stuff too frequently.
section
(maybegroup
otherwise I'm happy with section)Raxx.Logger
needs upgrading to be a runtime middleware. (Do this later by extracting the one I have in private project)Do we want a path and method specific match. i.e. (My opinion is just stick with match as is, now we have the option to experiment with other routing DSL's this is not so important)
Other Projects
Ace.HTTP.Service
instead anstart_link
function should be part of the generated code that incorporates the required middleware. (will complicate initial code but including some middleware is the right thing to do now we have it)WWW.Router
module reservingWWW
for start function and middleware config? (Maybe see if we like separate router in private project)use Ace.HTTP.Service
should not generate a start_link function. It no longer includes the server behaviour and is unaware of middleware (Ace generates start_link etc but they are overridable, see. https://github.com/CrowdHailer/Ace/pull/126)Previous notes
I was convinced this was not the best set of changes
This updates what the router understands.
Items in list of routes can be:
{match, controller}
(same as before){match. controller, function_name}
where function name is a function in the module that takes a configuration and returns a list of middleware.There are certainly nicer API's however this fulfils the goal of being middleware aware, without any breaking changes. It allows us to test middleware and come up with a nicer DSL later.
Also the separation of the Macro that builds the
route/2
function from where it is called suggests to me we might be able to make a base router, with no macro and that just lets the user implement the route function however they want and a full router that has a DSL for building function heads forroute/2