Implemented 405 response instead of 404, resolves #25. #37

WorstOfAny · 2017-09-04T11:06:26Z

No description provided.

codecov · 2017-09-04T11:08:12Z

Codecov Report

Merging #37 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #37   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          23     23           
  Lines         686    685    -1     
=====================================
- Hits          686    685    -1

Impacted Files	Coverage Δ
lib/flame/dispatcher.rb	`100% <100%> (ø)`	⬆️
lib/flame/dispatcher/routes.rb	`100% <100%> (ø)`	⬆️
lib/flame/router.rb	`100% <100%> (ø)`	⬆️
lib/flame/router/routes.rb	`100% <100%> (ø)`	⬆️
lib/flame/path.rb	`100% <100%> (ø)`	⬆️

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 15dd686...2bea737. Read the comment docs.

AlexWayfer · 2017-09-04T15:36:13Z

lib/flame/dispatcher/routes.rb

 				return nil unless route
-				status 200
+				status(200)


AlexWayfer · 2017-09-04T15:36:27Z

lib/flame/path.rb

@@ -11,7 +11,7 @@ class Path
 		## @param parts [Array<String, Flame::Path>] parts of expected path
 		## @return [Flame::Path] path from parts
 		def self.merge(*parts)
-			parts.join('/').gsub(%r{\/{2,}}, '/')
+			parts.join('/').gsub(%r[/{2,}], '/')


because shielding not needed if you use %r[]

pry: main > '/foo//bar///'.gsub(%r[/{2,}], '/') => "/foo/bar/" pry: main > '/foo//bar///'.gsub(%r{/{2,}}, '/') => "/foo/bar/"

???

Can you give proof, please?

oh, i meant that %r{\/} same that %r{/}

OK. So, why did you change the brackets?

because r%[/{2,}] looks more understandable than r%{/{2,}}

Ruby Style Guide recommends %r|...| for cases with {} inside Regexp.

Got it. Changed.

AlexWayfer · 2017-09-04T15:40:08Z

spec/unit/router_spec.rb

-				.should.equal nil
-		end
-	end
-


Add tests for Routes#endpoint, please.

AlexWayfer · 2017-09-04T15:43:40Z

lib/flame/router.rb

-				break if route || path_parts.empty?
-				path_parts.pop
+				route = routes.endpoint(*path_parts)&.values&.first
+				break if (route&.is_a?(Flame::Router::Route)) || path_parts.pop.nil?


Remove external parentheses, please.

AlexWayfer · 2017-09-04T15:46:37Z

lib/flame/dispatcher.rb

@@ -155,7 +156,7 @@ def cached_tilts
 		## Return response if HTTP-method is OPTIONS
 		def try_options
 			return unless request.http_method == :OPTIONS
-			allow = @app_class.router.routes.dig(request.path)&.allow
+			allow = @endpoint&.allow


Write tests for case with OPTIONS on route with optional parameters and without their values in path.

Before that PR it would not work, as I understand it, but now it will be.

works as you expected. Done

AlexWayfer · 2017-09-04T15:48:23Z

lib/flame/dispatcher.rb

@@ -29,6 +29,7 @@ def initialize(app_class, env)
 			@env = env
 			@request = Flame::Dispatcher::Request.new(env)
 			@response = Flame::Dispatcher::Response.new
+			@endpoint = @app_class.router.routes.endpoint(*request.path.parts)


Maybe lazy-loading via getter-method? And more obvious name, like available_endpoint, for example.

as you wish!

Just for you understanding: there is no point in wasting resources to find a route if static requested.

AlexWayfer · 2017-09-04T15:53:16Z

lib/flame/dispatcher/routes.rb

+				return nil unless @endpoint
+				allow = @endpoint.allow
+				halt(405, nil, 'Allow' => allow) if allow !~ /\b#{http_method}\b/
+				route = @endpoint[http_method]


Maybe halt 405 will be better after trying to find a route by specific HTTP-method? If route not found.

@endpoint existence meaning that there is a nested routes or route here. so if we are not allowed for a HTTP-method, why should we to try find route with this method? allow is returning HTTP-methods, that exist, so, if there is not allowed HTTP-method , why should we try find route by him?

Allow is the server response header about what HTTP-methods are allowed, in case the requested HTTP-method is not found. Routes#allow returns a string composed of the available keys, and then you search for the item in this string — this is a strange "back and forth" action.

I see a logic like this: there is a request, there is the Dispatcher#try_route method, if the route not found with requested HTTP-method, but there are others with it — error (halt) with Allow header.

You first build a halt, and then check how much it's appropriate. It seems to me strange and not very obvious (perhaps not very simple).

if the route not found with requested HTTP-method, but there are others

i trying to say that available_endpoint existence means that there is a routes or route. So we know, that there is a route or routes and have to see whether the HTTP-method is allowed. If HTTP-method not allowed then 405, else - execute route

i trying to say that available_endpoint existence means that there is a routes or route.

Route is not possible there. Routes or nil.

I'm trying to say that code without Regexp may be simpler :)

allow = available_endpoint.allow halt(405, nil, 'Allow' => allow) if allow !~ /\b#{http_method}\b/ status 200 execute_route available_endpoint[http_method]

vs.

route = available_endpoint[http_method] halt(405, nil, 'Allow' => available_endpoint.allow) unless route status 200 execute_route(route)

Uh. oK. understood. Changed.

AlexWayfer · 2017-09-05T11:18:01Z

lib/flame/dispatcher.rb

+		def available_endpoint
+			@available_endpoint ||=
+				@app_class.router.routes.endpoint(*request.path.parts)
+		end


You will try to find endpoint in each method calling if it not found :(

AlexWayfer · 2017-09-05T11:32:27Z

spec/unit/router/routes_spec.rb

+		should 'return nested routes from path' do
+			routes = @init.call(['/foo', '/:?var', '/bar'])
+			routes.endpoint('/foo').should.equal routes['foo'][':?var']
+			routes.endpoint('/foo/:?').should.equal routes['foo'][':?var']


Why '/foo/:?'?

AlexWayfer · 2017-09-05T11:55:13Z

lib/flame/dispatcher.rb

+		## Available routes endpoint
+		def available_endpoint
+			return @available_endpoint if defined? @available_endpoint
+			@available_endpoint ||=


There is no need to use || if there is defined?

yeah. changed.

snuggs · 2018-03-09T13:24:24Z

AWESOME work @AlexWayfer @WorstOfAny!

AlexWayfer · 2018-03-09T17:47:24Z

@snuggs thank you!

AlexWayfer requested changes Sep 4, 2017

View reviewed changes

WorstOfAny force-pushed the implement_405_response_instead_of_404 branch 5 times, most recently from 151af6f to faa803e Compare September 5, 2017 11:10

AlexWayfer requested changes Sep 5, 2017

View reviewed changes

WorstOfAny force-pushed the implement_405_response_instead_of_404 branch from faa803e to 87f0394 Compare September 5, 2017 11:45

AlexWayfer requested changes Sep 5, 2017

View reviewed changes

Implemented 405 response instead of 404, resolves AlexWayfer#25.

2bea737

WorstOfAny force-pushed the implement_405_response_instead_of_404 branch from 87f0394 to 2bea737 Compare September 5, 2017 11:57

AlexWayfer approved these changes Sep 5, 2017

View reviewed changes

AlexWayfer merged commit b1cf7e3 into AlexWayfer:master Sep 5, 2017

snuggs mentioned this pull request Mar 9, 2018

HTTP Resource Representation Variants devpunks/snuggsi#164

Merged

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented 405 response instead of 404, resolves #25. #37

Implemented 405 response instead of 404, resolves #25. #37

WorstOfAny commented Sep 4, 2017

codecov bot commented Sep 4, 2017 •

edited

Loading

AlexWayfer Sep 4, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 4, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 5, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 5, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 5, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 4, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 4, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 4, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 4, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 5, 2017

AlexWayfer Sep 4, 2017

WorstOfAny Sep 5, 2017 •

edited

Loading

AlexWayfer Sep 5, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 5, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 5, 2017

WorstOfAny Sep 5, 2017

AlexWayfer Sep 5, 2017 •

edited

Loading

WorstOfAny Sep 5, 2017

AlexWayfer Sep 5, 2017

WorstOfAny Sep 5, 2017

snuggs commented Mar 9, 2018

AlexWayfer commented Mar 9, 2018

Implemented 405 response instead of 404, resolves #25. #37

Implemented 405 response instead of 404, resolves #25. #37

Conversation

WorstOfAny commented Sep 4, 2017

codecov bot commented Sep 4, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WorstOfAny Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWayfer Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snuggs commented Mar 9, 2018

AlexWayfer commented Mar 9, 2018

codecov bot commented Sep 4, 2017 •

edited

Loading

WorstOfAny Sep 5, 2017 •

edited

Loading

AlexWayfer Sep 5, 2017 •

edited

Loading