Adding the optional routing in the to core

[jwoertink]

Purpose

Optional path params were added in to the router luckyframework/lucky_router#18 but there was never a test done to see if Lucky core needed any updates. I added a spec to see if we needed to do anything, and it doesn't look like it.

Checklist

• - An issue already exists detailing the issue/or feature request that this PR fixes
• - All specs are formatted with crystal tool format spec src
• - Inline documentation has been added and/or updated
• - Lucky builds on docker with ./script/setup
• - All builds and specs pass on docker with ./script/test

[paulcsmith]

I believe it does still need additional specs for accessing the param. If you try to access the optional param it needs to be nilable but right now I don't believe it is. It would likely raise a NilAssertionError if the param is missing from the path :(

[jwoertink]

Ah, gotcha. Ok, I'll take a look at that!

[paulcsmith]

Nice! I believe this is the spot:

lucky/src/lucky/routable.cr

Lines 198 to 203 in 72336b7

	{% if part.starts_with?(":") %}
	{% part = part.gsub(/:/, "").id %}
	def {{ part }} : String
	params.get(:{{ part }})
	end
	{% end %}

. You'd want to check for starts_with?("?:") and use params.get?(). I think it'll also need to update the with and route helpers so that the params can be nil and they have a default value of nil which is somewhere in here I think :P

lucky/src/lucky/routable.cr

Lines 228 to 243 in 72336b7

	{% for param in path_params %}
	{{ param.gsub(/:/, "").id }},
	{% end %}
	{% params_with_defaults = PARAM_DECLARATIONS.select do |decl|
	decl.value || decl.type.is_a?(Union) && decl.type.types.last.id == Nil.id
	end %}
	{% params_without_defaults = PARAM_DECLARATIONS.reject do |decl|
	params_with_defaults.includes? decl
	end %}
	{% for param in params_without_defaults + params_with_defaults %}
	{% is_nilable_type = param.type.is_a?(Union) %}
	{% no_default = !param.value && param.value != false && param.value != nil %}
	{{ param }}{% if is_nilable_type && no_default %} = nil{% end %},
	{% end %}
	anchor : String? = nil

[jwoertink]

😂 ok, so now that I just merged in that Avram update, this is failing. Should I include those Avram updates in this? Or make a separate PR to update that, then get that PR merged in to here? 🤔

[paulcsmith]

@jwoertink I think a separate PR with Avram fixes would be good since it’s small and we can get that in super fast

[jwoertink]

@paulcsmith Alright, I sent in that other PR and merged it in. It was a pretty small change. Let me know if you see any cases missing on this one.

[paulcsmith]

I think that covers everything. LGTM!