search

luarocks / luarocks-site

LuaRocks website and module host
http://luarocks.org
176 stars 36 forks source link

Allow to use json or zip format without lua version in manifest resources #77

Closed jirutka closed 8 years ago

jirutka commented 8 years ago

To be more specific, the following resources has been added:

jirutka commented 8 years ago

@leafo gentle ping

leafo commented 8 years ago

Sorry for the late response, been a little busy (as always).

There are some issues with how you did the splat matching. The splat is way too greedy, and also moonscript style routes have no guaranteed ordering, so you'll get some funky results

Here are a few examples:

http://localhost.com:8080/manifest/hello renders 200, should be 404 http://localhost.com:8080/manifestfefefe renders 200, matches /manfest*, but should be 400 http://localhost.com:8080/manifests/leafo/manifest matches /manifest* instead of my manifest

jirutka commented 8 years ago

Uh, then I really don’t know how to implement this properly on Lapis. :( It lacks optional parameters.

leafo commented 8 years ago

alright, I can make the patch then

jirutka commented 8 years ago

What if I replace e.g. /manifest* with /manifest:rest and @params.spread with @params.rest, would it work better…? However, it’s perhaps even uglier workaround than spread.

leafo commented 8 years ago

That sounds like it would work

jirutka commented 8 years ago

http://localhost.com:8080/manifest/hello renders 200, should be 404 http://localhost.com:8080/manifestfefefe renders 200, matches /manfest*, but should be 400

This should actually work correctly, but I’ve messed the following line:

if not valid_format @format or splat_rest == ""

The second part of the condition is wrong, there should be splat_rest ~= "". However, it’s still wrong, because… omitting braces is sometimes tricky… It should be:

if not valid_format(@format) or splat_rest ~= ""

Sorry for this mistake.

http://localhost.com:8080/manifests/leafo/manifest matches /manifest* instead of my manifest

This problem is indeed caused by greedy behaviour of the splat operator.

I’ve fixed the mentioned problems, so please give it a second try.

leafo commented 8 years ago

looks like we missed one, the url /manifests should match this page: https://luarocks.org/manifests I added a bunch of specs identifying pages that shouldn't match.

I know this routing stuff is a pain, I opened an issue leafo/lapis#384 to help fix these problems.

I'm just going to patch lapis for this, to give precedence to routes that match exactly

leafo commented 8 years ago

manually merged in after some updates, thanks for the patch