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

RFC: revert "revert remove HashDict" #252

Merged
merged 3 commits into from
Jan 17, 2017
Merged

Conversation

kmsquire
Copy link
Member

@kmsquire kmsquire commented Jan 15, 2017

This removes HashDict again, and fixes some issues uncovered by basing DefaultDicts on Base.Dict rather than HashDict.

Previously, getindex always called get!(h::Associative, key, default) to return a default value.

When DefaultDict was based on HashDict (now deleted) and the default value was calculated using a function, a version of this function with signature

get!(h::HashDict, key, default::Base.Callable)

was called.

Base.Dict has no such function (nor does DataStructures.OrderedDict), so the (incorrect) value returned was the function object which generated the default value, not the default value itself.

Additionally, there were no tests for DefaultDicts with defaults generated by a function.

  • Fixed so that getindex calls the do function form of get! on DefaultDicts/DefaultOrderedDicts when appropriate
  • Added tests for DefaultDicts with defaults generated by functions

Edit: Also, add back functionality for pushing tuples to DefaultDicts.

@codecov-io
Copy link

codecov-io commented Jan 15, 2017

Current coverage is 95.98% (diff: 100%)

Merging #252 into master will increase coverage by 3.06%

@@             master       #252   diff @@
==========================================
  Files            31         30     -1   
  Lines          2486       2192   -294   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           2310       2104   -206   
+ Misses          176         88    -88   
  Partials          0          0          

Powered by Codecov. Last update 28c5f3f...e1ae53e

@tkelman
Copy link
Contributor

tkelman commented Jan 15, 2017

This fixes the Gadfly problem I hope?

* Previously, `getindex` always called "get!(h::Associative, key, default)"
  to return a default value.
* When DefaultDict was based on HashDict (now deleted) and the
  default value was calculated using a function, a version of this
  function with signature

     get!(h::HashDict, key, default::Base.Callable)

  was called.
* Base.Dict has no such function (nor does DataStructures.OrderedDict),
  so the (incorrect) value returned was the function object which
  generated the default value, not the default value itself.
* Additionally, there were no tests for DefaultDicts with defaults
  generated by a function.

* Fixed so that getindex calls the `do` function form of `get!` when
  appropriate
* Added tests for DefaultDicts with defaults generated by functions
@kmsquire kmsquire changed the title WIP: revert remove HashDict WIP: revert "revert remove HashDict" Jan 17, 2017
@kmsquire kmsquire changed the title WIP: revert "revert remove HashDict" RFC: revert "revert remove HashDict" Jan 17, 2017
@kmsquire kmsquire merged commit f322f20 into master Jan 17, 2017
@kmsquire kmsquire deleted the kms/revert_remove_hashdict branch July 13, 2017 20:13
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.

None yet

3 participants