Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add query param to path even if default value is passed #1239

Merged

Conversation

matthewmcgarvey
Copy link
Member

@matthewmcgarvey matthewmcgarvey commented Aug 19, 2020

Purpose

Fixes #1201

When attempting to get a path, if you happened to pass in a value to a query param that matched the default value, it was not added to the resulting url.

Description

This change removes all references to the default value of a param when building a path.

Here is a comparison of the route method output. It is formatted and has comments removed.

Before

def self.route(
  no_default : String,
  has_default : String = "default",
  has_nil_default : String | ::Nil = nil,
  anchor : String? = nil
) : Lucky::RouteHelper
  path = path_from_parts()
  query_params = {} of String => String
  param_is_default_or_nil = has_default == "default"
  unless param_is_default_or_nil
    query_params["has_default"] = has_default.to_s
  end
  param_is_default_or_nil = has_nil_default == nil
  unless param_is_default_or_nil
    query_params["has_nil_default"] = has_nil_default.to_s
  end
  param_is_default_or_nil = no_default == nil
  unless param_is_default_or_nil
    query_params["no_default"] = no_default.to_s
  end

  unless query_params.empty?
    path += "?#{HTTP::Params.encode(query_params)}"
  end

  anchor.try do |value|
    path += "#"
    path += URI.encode_www_form(value)
  end

  Lucky::RouteHelper.new :get, path
end

After

def self.route(
  no_default : String,
  has_default = nil,
  has_nil_default = nil,
  anchor : String? = nil
) : Lucky::RouteHelper
  path = path_from_parts()
  query_params = {} of String => String
  query_params["has_default"] = has_default.to_s unless has_default.nil?
  query_params["has_nil_default"] = has_nil_default.to_s unless has_nil_default.nil?
  query_params["no_default"] = no_default.to_s unless no_default.nil?
  
  unless query_params.empty?
    path += "?#{HTTP::Params.encode(query_params)}"
  end

  anchor.try do |value|
    path += "#"
    path += URI.encode_www_form(value)
  end

  Lucky::RouteHelper.new :get, path
end

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

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting! These macros are a beast to figure out so am not 100% sure how this is working. Do you think you could leave some comments about what it is doing and what the data looks like?

Also one suggestion to add a spec to make sure the defaults are not included if not given explicitly

Comment on lines 353 to 356
it "is added to the path even if the value matches default" do
OptionalParams::Index.path(with_default: "default").should eq "/optional_params?with_default=default"
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a spec for this that makes sure that the default is not shown if not explicitly given? I'm imagining something like this:

Suggested change
it "is added to the path even if the value matches default" do
OptionalParams::Index.path(with_default: "default").should eq "/optional_params?with_default=default"
end
it "is added to the path if the value matches default and is explicitly given" do
OptionalParams::Index.path(with_default: "default").should eq "/optional_params?with_default=default"
end
it "is not added to the path if not explicitly given" do
OptionalParams::Index.path(with_default: "default").should eq "/optional_params"
end

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you :D

@paulcsmith paulcsmith merged commit 229311a into luckyframework:master Aug 20, 2020
@matthewmcgarvey matthewmcgarvey deleted the default-query-params-fix branch August 20, 2020 13:20
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow override for displaying default parameters in path URL?
2 participants