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

Evaluator#key? forwards key name argument #664

Merged
merged 1 commit into from
Aug 21, 2020

Conversation

alassek
Copy link
Contributor

@alassek alassek commented Aug 11, 2020

When testing the presence of multiple, optional keys, it would be nice to be able to specify the key name to key? as you can with values.key?.

Since this merely delegates to that method, I can't see any reason why this omission was intentional. #key_name is preserved as the default argument, so this will not change any existing behavior.

discussed in the forum here

When testing the presence of multiple, optional keys, it would be nice
to be able to specify the key name to `key?` as you can with
`values.key?`.

Since this merely delegates to that method, I can't see any reason why
this omission was intentional. `#key_name` is preserved as the default
argument, so this will not change any existing behavior.
Copy link
Contributor

@esparta esparta 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 this, at some point have the same question but you got the solution!

Just wondering about contract level scenario, later I can provide some rspec to complement this if.

The `key?` method supports passing an explicit key name for rules that have multiple keys.

```ruby
class DistanceContract < Dry::Validation::Contract
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a spec at contract level (/spec/integration/contract/) mimicking this same scenario where we are asking of an specific keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I think the test coverage is pretty comprehensive already but that would probably better communicate the intent of this.

Copy link
Member

Choose a reason for hiding this comment

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

I merged it as-is because having this example in the docs already covers the intended usage and the implementation is so simple that the unit test will suffice.

# @return [Boolean]
#
# @api public
def key?
values.key?(key_name)
def key?(name = key_name)
Copy link
Member

@solnic solnic Aug 12, 2020

Choose a reason for hiding this comment

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

I think we want to support the same type of args as Values#key?, WDYT? Values supports symbols, arrays, path objects etc. so this could be something like:

def key?(*args)
  args.empty? ? values.key?(key_name) : values.key?(*args)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was intending this to work with Schema::Path, yes.

The Values implementation doensn't rely on splatting to work, is that necessary here?

      def key?(key, hash = data)
        return hash.key?(key) if key.is_a?(Symbol)

        Schema::Path[key].reduce(hash) do |a, e|
          if e.is_a?(Array)
            result = e.all? { |k| key?(k, a) }
            return result
          elsif e.is_a?(Symbol) && a.is_a?(Array)
            return false
          else
            return false unless a.is_a?(Array) ? (e >= 0 && e < a.size) : a.key?(e)
          end
          a[e]
        end

        true
      end

Copy link
Member

Choose a reason for hiding this comment

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

@alassek ah right, doesn't have to be *args, good point!

Copy link
Member

@solnic solnic left a comment

Choose a reason for hiding this comment

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

One small thing to fix in the docsite, otherwise we can :shipit: 😄

class DistanceContract < Dry::Validation::Contract
schema do
optional(:kilometers).filled(:integer)
optional(:miles).filled(:integer)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
optional(:miles).filled(:integer)
optional(:miles).value(:integer)

```ruby
class DistanceContract < Dry::Validation::Contract
schema do
optional(:kilometers).filled(:integer)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
optional(:kilometers).filled(:integer)
optional(:kilometers).value(:integer)

@solnic solnic merged commit 8f63c44 into dry-rb:master Aug 21, 2020
@alassek
Copy link
Contributor Author

alassek commented Aug 21, 2020

Sorry, was not blowing this off just had a busy week. Thanks for fixing the doc example for me!

@solnic
Copy link
Member

solnic commented Aug 24, 2020

@alassek no worries!

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