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

Add method `Amber::Validators::Params#to_unsafe_h` #1043

Merged
merged 2 commits into from Jan 30, 2019

Conversation

Projects
None yet
6 participants
@westonganger
Copy link
Contributor

commented Jan 15, 2019

Solves #1042

PR for documentation to be added after.

@westonganger westonganger force-pushed the westonganger:patch-6 branch from 6661ebb to 29bddf3 Jan 15, 2019

@robacarp robacarp requested a review from eliasjpr Jan 16, 2019

@robacarp
Copy link
Member

left a comment

@eliasjpr what do you think?

@drujensen

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

needs at least one spec.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Spec added

@westonganger westonganger referenced this pull request Jan 16, 2019

Merged

Improve Params Docs #134

@drujensen

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

I'm hesitant to add this since it is dangerous to allow unexpected params to be processed. I agree with the name of the method as to_unsafe_h but worried this may be abused and expose a way to bypass the safety net.

I am interested what @eliasjpr as well.

@eliasjpr

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

Im fine with this. I would rather have people to call raw_params.to_h themselves.

@anamba

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2019

I've been running my apps with a local patch for a while, with the intention of someday opening a PR, just never got around to it. Looks like @westonganger beat me to it, so I'm adding my two cents now.

Here's my patch:

module Amber::Validators
  class Params
    def to_raw_h
      raw_params.to_h
    end

    def to_unsafe_h
      unsafe_params = {} of String => String?

      @rules.each do |rule|
        rule.apply(raw_params)
        unsafe_params[rule.field] = rule.value
      end

      unsafe_params
    end
  end
end

Looks like @westonganger 's PR is just doing what I call #to_raw_h, which I rarely use (for the reason @drujensen mentioned, you never know what you're gonna get).

So I have a second method, my version of #to_unsafe_h (which is actually somewhat more safe). It still gives you params that haven't been fully validated, but at least you it only gives you the params you've declared (via required and optional).

@drujensen drujensen merged commit 593bb6f into amberframework:master Jan 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@anamba

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

Guess I'd better rename my own #to_unsafe_h to avoid clobbering this new one.

Perhaps I'll re-implement as a boolean flag to #to_h that would allow it to return the values of all declared params, even those that have not passed validation. I would use it to update the model object in memory, to populate the form after an invalid submission (the user shouldn't have to re-enter everything from scratch).

Let me know if that sounds useful and I can open a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.