Add upload file multiple support, fix more problem about route scope. #1121
Conversation
| filename = upload.filename | ||
| if filename.is_a?(String) && !filename.empty? | ||
| files[upload.name] = Amber::Router::File.new(upload: upload) | ||
| if files.has_key? upload.name |
There was a problem hiding this comment.
I'm struggling to follow this logic. Can you refactor this down into smaller methods?
There was a problem hiding this comment.
I agree we can separate these?
There was a problem hiding this comment.
I'll trying to refactor it. Thank you.
wndfly
force-pushed
the
master
branch
3 times, most recently
from
8faf529
to
393adea
Compare
| @@ -12,4 +12,10 @@ module Amber::DSL::Server | |||
| end | |||
| {% end %} | |||
| end | |||
|
|
|||
| macro namespace(scoped_namespace, value = :web) | |||
There was a problem hiding this comment.
This is a duplicated from https://github.com/amberframework/amber/pull/1121/files#diff-3fe37131d1005de63b8c08c6ecbbcb03R2
You just swapped valve and scope.
There was a problem hiding this comment.
I swapped them because I want used it like this.
namespace "/foo" do
namespace "/bar" do
end
end
namespace "/foo1", :web do
namespace "/bar" do
end
end
There was a problem hiding this comment.
And what happen for
routes :web do
namespace "/foo", :api do
get "/bar" ...
end
end
This is inconsistant and allow developpers to do crappy things.
src/amber/router/scope.cr
Outdated
| @@ -1,9 +1,9 @@ | |||
| module Amber | |||
| module Router | |||
| class Scope | |||
| @stack = [] of String | |||
| property :stack | |||
There was a problem hiding this comment.
Why leak the internal into the external world ?
No one should know Scope use an Array
| @@ -25,13 +24,11 @@ module Amber | |||
|
|
|||
| # This registers all the routes for the application | |||
| def draw(valve : Symbol) | |||
| with DSL::Router.new(self, valve, scope) yield | |||
| with DSL::Router.new(self, valve, Scope.new) yield | |||
There was a problem hiding this comment.
Using Scope.new, you lose all benefits of nested scopes.
| scope.push(namespace) | ||
| with DSL::Router.new(self, valve, scope) yield | ||
| scope.pop | ||
| with DSL::Router.new(self, valve, Scope.new([namespace])) yield |
There was a problem hiding this comment.
Why not use the current scope ?
There was a problem hiding this comment.
When I used amber, I discovered a problem at the method with route match which doesn't add scope, so I modified more about scope.
Maybe I should modify it here, But I think keep scope in every route might be better.
…aram name consistent with others. add ignore. fix a bug at route scope. fix a bug about static file with route scope. fix namespce route config.
| @@ -28,7 +28,6 @@ module Amber::DSL | |||
| macro namespace(scoped_namespace) | |||
| scope.push({{scoped_namespace}}) | |||
| {{yield}} | |||
| scope.pop | |||
There was a problem hiding this comment.
You push but not pop, this will add more and more namespaces.
|
@wndfly can you add the description to the PR and follow the template this helps the team evaluate the changes and intentions behind the changes. For instance I am not sure I understand the benefits of having then namespace namespace "/foo" do
# ... routes here
namespace "/bar" do
# ... routes here
end
endversus repeating scopes scope "/foo", :web do
# ... routes here
end
scope "/foo/bar", :web do
# ... routes here
end
Having the ability to upload multiple files is 💯 good with me, it would be great to have them on a separate PR (the files part would have already been merged by now). |
|
I'm going to close this PR. The file upload part would be nice as a separate PR. Mixing the routing change to the namespace is killing this PR. |
Description of the Change
Alternate Designs
Benefits
Possible Drawbacks