Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Working out Array param stuff #1527

Merged
merged 4 commits into from
Jun 17, 2021
Merged

Working out Array param stuff #1527

merged 4 commits into from
Jun 17, 2021

Conversation

jwoertink
Copy link
Member

@jwoertink jwoertink commented Jun 10, 2021

Purpose

Alternative to #1519

Description

Going the route of #1519 could still work; however, that route requires that params are defined in separate objects. In larger projects, that could end up being lots of duplication and such.

This first commit takes some of that code used in params, but with the idea that we leave the param parsing in Avram Operations. The plus side to this path is there's no breaking changes. We could support a few additional types with Arrays which would make forms slightly more robust. The downsides are that we leave the avram/lucky coupling, testing params in Avram become a little more difficult as we now have to add more methods to paramable, and this path still doesn't get us closer to supporting model associations (i.e. accepts_nested_attributes_for, etc...)

Maybe this is just a stepping stone of progress towards making some of those downsides doable?

This PR now makes this possible:

class Searches::Index < BrowserAction
  param tags : Array(String)

  get "/searches" do
    results = SearchStuff.run!(tags: tags)

    html IndexPage, results: results
  end
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


params = Lucky::Params.new(request)

params.nested_arrays?(:user).should eq({"langs" => ["ruby", "elixir"]})
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is eq not includes shouldnt "name" => "paul" be in the params hash?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one was a little tricky. The method only returns the nested arrays, it doesn't return any other keys. This is similar to how the file param stuff works.

The reason why is because I either had to turn the non-array pairs in to "name" => ["paul"] to keep the single Hash(String, Array(String)) type, or, have the value be a union in which case calling methods on the values becomes a little more difficult. If we merged all of them in to a single method, then the type signature becomes pretty gnarly.

@jwoertink jwoertink changed the title [WIP] Working out Array param stuff Working out Array param stuff Jun 16, 2021
@jwoertink
Copy link
Member Author

I was originally going to look at also dealing with files and arrays of files in this, but then I started thinking that it's probably a super rare case where you would set a file that's not nested in your action. Any file handling you would do will most likely be done in an operation (which still doesn't work). The param macro in actions seems to be for query params most often. However, with this code setup, we could add that in for a separate PR if it's something people want.

@jwoertink jwoertink merged commit e00e143 into master Jun 17, 2021
@jwoertink jwoertink deleted the experimental/all_the_params branch June 17, 2021 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants