Fix StrictHash contract for extra keys #254

Merged
merged 2 commits into from Apr 20, 2017

Conversation

Projects
None yet
2 participants
@smt116
Contributor

smt116 commented Apr 7, 2017

Contracts::StrictHash didn't complain about extra entries in a given
hash. This was because of the missing assertion for keys. Specs hadn't
caught this case because age key had a wrong type anyway.

Compare keys for contract and a given hash and return false if they are
equal.

@@ -405,6 +405,7 @@ def initialize(contract_hash)
def valid?(arg)
return false unless arg.is_a?(Hash)
+ return false unless arg.keys.sort.eql?(contract_hash.keys.sort)

This comment has been minimized.

@smt116

smt116 Apr 7, 2017

Contributor

sort is missing for ruby 1.8.7, but is there any point of supporting this version? Also what do you think about comparing arrays sizes instead?

@smt116

smt116 Apr 7, 2017

Contributor

sort is missing for ruby 1.8.7, but is there any point of supporting this version? Also what do you think about comparing arrays sizes instead?

This comment has been minimized.

@egonSchiele

egonSchiele Apr 18, 2017

Owner

I think we can drop support for ruby 1.8.7, I like comparing the keys instead of array size though.

@egonSchiele

egonSchiele Apr 18, 2017

Owner

I think we can drop support for ruby 1.8.7, I like comparing the keys instead of array size though.

lib/contracts/builtin_contracts.rb
@@ -405,6 +405,7 @@ def initialize(contract_hash)
def valid?(arg)
return false unless arg.is_a?(Hash)
+ return false unless arg.keys.sort.eql?(contract_hash.keys.sort)
contract_hash.all? do |key, _v|
contract_hash.key?(key) && Contract.valid?(arg[key], contract_hash[key])

This comment has been minimized.

@smt116

smt116 Apr 7, 2017

Contributor

is there any point of accessing contract_hash[key] instead of using _v from the iterator? (which should be renamed to contract in such case)

@smt116

smt116 Apr 7, 2017

Contributor

is there any point of accessing contract_hash[key] instead of using _v from the iterator? (which should be renamed to contract in such case)

This comment has been minimized.

@egonSchiele

egonSchiele Apr 18, 2017

Owner

nope, that sounds like a good change to me

@egonSchiele

egonSchiele Apr 18, 2017

Owner

nope, that sounds like a good change to me

@egonSchiele

This comment has been minimized.

Show comment
Hide comment
@egonSchiele

egonSchiele Apr 16, 2017

Owner

Thanks for this PR! I will check it shortly

Owner

egonSchiele commented Apr 16, 2017

Thanks for this PR! I will check it shortly

lib/contracts/builtin_contracts.rb
@@ -405,6 +405,7 @@ def initialize(contract_hash)
def valid?(arg)
return false unless arg.is_a?(Hash)
+ return false unless arg.keys.sort.eql?(contract_hash.keys.sort)
contract_hash.all? do |key, _v|
contract_hash.key?(key) && Contract.valid?(arg[key], contract_hash[key])

This comment has been minimized.

@egonSchiele

egonSchiele Apr 18, 2017

Owner

nope, that sounds like a good change to me

@egonSchiele

egonSchiele Apr 18, 2017

Owner

nope, that sounds like a good change to me

@@ -405,6 +405,7 @@ def initialize(contract_hash)
def valid?(arg)
return false unless arg.is_a?(Hash)
+ return false unless arg.keys.sort.eql?(contract_hash.keys.sort)

This comment has been minimized.

@egonSchiele

egonSchiele Apr 18, 2017

Owner

I think we can drop support for ruby 1.8.7, I like comparing the keys instead of array size though.

@egonSchiele

egonSchiele Apr 18, 2017

Owner

I think we can drop support for ruby 1.8.7, I like comparing the keys instead of array size though.

@smt116

This comment has been minimized.

Show comment
Hide comment
@smt116

smt116 Apr 18, 2017

Contributor

Fixed. I didn't touched 1.8.7 support as I saw that someone else wanted to do that (#255 (comment)).

Contributor

smt116 commented Apr 18, 2017

Fixed. I didn't touched 1.8.7 support as I saw that someone else wanted to do that (#255 (comment)).

@egonSchiele

This comment has been minimized.

Show comment
Hide comment
@egonSchiele

egonSchiele Apr 20, 2017

Owner

can you merge master into this branch? ci should pass, and then I'll merge

Owner

egonSchiele commented Apr 20, 2017

can you merge master into this branch? ci should pass, and then I'll merge

smt116 added some commits Apr 7, 2017

Fix StrictHash contract for extra keys
`Contracts::StrictHash` didn't complain about extra entries in a given
hash. This was because of the missing assertion for keys. Specs hadn't
caught this case because `age` key had a wrong type anyway.

Compare keys for contract and a given hash and return false if they are
equal.
Do not use a second lookup for the contract in StrictHash assertion
`all?` iterator already provides contract for the `valid?` check. Use
it instead of accessing `contract_hash` again.
@smt116

This comment has been minimized.

Show comment
Hide comment
@smt116

smt116 Apr 20, 2017

Contributor

@egonSchiele done.

Contributor

smt116 commented Apr 20, 2017

@egonSchiele done.

@egonSchiele egonSchiele merged commit c408bcc into egonSchiele:master Apr 20, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@egonSchiele

egonSchiele Apr 20, 2017

Owner

Thank you!

Owner

egonSchiele commented Apr 20, 2017

Thank you!

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Jun 4, 2017

taca
Update ruby-contracts to 0.16.0.
## v0.16.0

- **Support for Ruby 1.8 has been discontinued** - [Corey Farwell](https://github.com/frewsxcv) [#256](egonSchiele/contracts.ruby#256)
- Enhancement: Add a `Contracts::Attrs` module containing attribute w/ contracts utilities - [Corey Farwell](https://github.com/frewsxcv) [#255](egonSchiele/contracts.ruby#255)
- Bugfix: Fix StrictHash contract for extra keys - [Maciej Malecki](https://github.com/smt116) [#254](egonSchiele/contracts.ruby#254)

## v0.15.0
- Bugfix: Func contract's return value isn't enforced with blocks - [Piotr Szmielew](https://github.com/esse) [#251](egonSchiele/contracts.ruby#251)
- Bugfx: Fix contracts used in AR-models - [Gert Goet](https://github.com/eval) [#237](egonSchiele/contracts.ruby#237)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment