-
Notifications
You must be signed in to change notification settings - Fork 229
[Hash] Update Hash's signatures and tests to be correct #2694
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
base: master
Are you sure you want to change the base?
[Hash] Update Hash's signatures and tests to be correct #2694
Conversation
4394328 to
944d757
Compare
944d757 to
054c857
Compare
| type.literal == val | ||
| begin | ||
| type.literal == val | ||
| rescue NoMethodError | ||
| raise if defined?(val.==) | ||
| false | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the exact same snippet as https://github.com/ruby/rbs/pull/2683/files#diff-b00b4cb204913354ffdc33b64788f911df698b99c4c65bf6cfa6bbea17e2870a... If either that or this PR is merged, i'll rebase the other
|
Pushed a new version which retains |
| def self.try_convert: [U, V] (_ToHash[U, V]) -> ::Hash[U, V] | ||
| | (untyped) -> (::Hash[untyped, untyped] | nil) | ||
| def self.try_convert: [K, V] (hash[K, V] hash) -> Hash[K, V] | ||
| | (untyped) -> nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep the base case for situations that static type information of the argument is lost, but actually is a to_hash compatible object.
| | (untyped) -> nil | |
| | (untyped) -> Hash[untyped, untyped]? |
| # h[:nosuch] # => nil | ||
| # | ||
| def []: %a{implicitly-returns-nil} (K arg0) -> V | ||
| def []: %a{implicitly-returns-nil} (_Key key) -> V |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically correct but controversial.
I’m assuming it makes more sense to get a type error when we pass an unexpected value.
hash = { foo: :bar } #: Hash[Symbol, Symbol]
hash["foo"] # This is usually a mistake.|
Oops... I think having a few test cases especially around |
PHEW! This was a long one, but now
Hashis finally done!This PR updates all of the definitions within
hash.rbsto be more correct, as well as modernizes the entireHash_test.rbfile, which was still using the old format.A complete list of functional changes is below. Bolded ones are important.
Hash::_Pair[K, V]is added;Hash.[]andHash#to_huse it.Hash::_Equalsis added. Used byHash#{assoc,has_value?,rassoc}.Hash.[]has been changed to returninstances in theHash[a: 1]andHash[[:a, 1]]cases, and now uses_PairHash.try_convertreturnsnilin theuntypedcase.Hash#{<,>,<=,>=}all now accepthash[untyped, untyped]Hash#[]now acceptsHash::_KeyHash#any?'s signature has been fixed: the block form actually is a tuple, and now usesEnumerable::_PatternEnumerable::_Patternto take a genericHash#{assoc,rassoc,has_value?,key}now all takeHash::_EqualsHash#compactreturns aninstance. There's no real good way to represent the return value for the function, butinstanceis more accurate thanHash[K, V]Hash#deconstruct_keysnow has anuntypedargument, as it's ignoredHash#{default,dig,delete,except,fetch,fetch_values,has_key?,slice,values_at}now all acceptHash::_Key, and any related blocks also do as wellHash#default_procnow returns a(^(self, _Key) -> V)?. I'm not sure if thisselfis legal, or needs to be aninstance(or evenHash[K, V], though I don't like that because it doesnt account forHashsubclasses)Hash#default_proc=is similarly plagued withHash#default_proc's concern. Also, the three branches were split up.Hash#each{,_key,_value}'s blocks now returnsvoidnotuntypedHash#flattennow explicitly handles a level of0, and now acceptsintsHash#{merge,merge!}now both accepthash. Unfortunately, formergereturn type isHash, but really it should be "instance[A | K, B | V]"Hash#freezehas been addedHash#replacenow acceptshashsize/length,filter/select, andfilter!/select!have been swapped, to more closely match documentation. (Other aliases which matched the documentation haven't been touched)Hash#to_h's block form can now return_Pair[A, B]Hash#{transform_keys{,!}}variants with hash arguments ({a: 1}.transform_keys(a: :b) #=> {b: 1}) have been addedHash#initialize's proc now accepts aninstance