Update README to flag issue with optional routes. #1025
Conversation
| get '/posts.?:format?' do | ||
| # matches "GET /posts" and any extension "GET /posts.json", "GET /posts.xml" etc. | ||
| get '/post/:format?' do | ||
| # matches "GET /posts" and any extension "GET /posts/json", "GET /posts/xml" etc |
There was a problem hiding this comment.
I feel the description quite doesn't fit here since it looks like a path fragment rather than an extension. Perhaps this should be reworded?
There was a problem hiding this comment.
Additionally, the proposed change would not match "GET /posts", but "GET /post/".
How about something that would be more typically used as a path fragment? Perhaps
get '/posts/?:year?' do
# matches "GET /posts" and any year "GET /posts/2009", "GET /posts/2010", etc
endThere was a problem hiding this comment.
get /posts/:year? would be ok but the the /? means the problem would remain, as it'd match posts_2005 as a :year of _2005
There was a problem hiding this comment.
Ah, right. Damn, I missed that.
OK, so if we do get '/posts/:year?, then the comment would have to say that it `matches "GET /posts/", which sucks as a route, since people typing things in would tend to leave off the trailing slash.
There was a problem hiding this comment.
I believe most HTTP servers automatically mismatch the trailing ‘/‘, which is how the /folder/index.html trick works as a /folder url (e.g. with middleman), or at least thats what I was lead to believe when I picked the example :)
There was a problem hiding this comment.
This is what I get:
$ cat app.rb
require 'sinatra'
get '/hello/' do
'Hello world!'
end
$ ruby app.rb
$ curl -v 'localhost:4567/hello/'
HTTP/1.1 200 OK
$ curl -v 'localhost:4567/hello/'
HTTP/1.1 404 Not Found
There was a problem hiding this comment.
Ah it's true webbrick doesn't but nginx and apache fronting it would, tricky.
There was a problem hiding this comment.
Oh, wow. I didn't realize that nginx/apache would treat these differently. Yeah, that is tricky :/
Thin behaves the same way as webrick. I didn't check puma and unicorn.
|
@mhutter left a comment in #1023 (comment) which I think is the right way to go for fixing the README. What do you think @JonRowe ? |
|
Sooo question here, is it really expected for the |
|
I actually tried that route at fist @kytrinyx but couldn't make it work, which is why I went down the rewording the readme to avoid the issue entirely route. |
|
I would actually leave the README as-is - It's only an example. Or replace by a completely generic scenario, such as get '/hello/:name?' do
name = params[:name] || "World!"
"Hello, #{name}"
endBut then again this example will behave differently depending on the webserver... |
|
Thats what this PR is about, changing the example to a more correct if more generic example, just to prevent confusion as in #1023 |
|
Anything I can do to move this along? |
The original example would also match `GET /posts_anything` as `GET /posts` with a format of `"_anything"`
|
We have to update the translations too, let me see if I can fix this. |
Update README to flag issue with optional routes.
The original example would also match
GET /posts_anythingasGET /postswith a format of"_anything"because of the optional format separator, forcing the separator is kind of an ugly example so switch to using a trailing slash which should be recognised either way (I think). It'd be nice to be able to create optional groups e.g.(.:format)?