Implement System.Collections.IDictionary for dict #436

forki · 2015-05-11T10:18:36Z

I tried to implement http://fslang.uservoice.com/forums/245727-f-language/suggestions/7845018-make-the-result-of-the-dict-function-implement-idi but got stuck at:

I assume this is only for vNext + 1

enricosada · 2015-05-11T10:59:26Z

@forki just return the sequence enumerator? why cast sequence of DictionaryEntry to IDictionary?

forki · 2015-05-11T11:16:20Z

It needs to return IDictionaryEnumerator

enricosada · 2015-05-11T11:21:11Z

i think the easier way is to create a class DictEnumerator, inherit from both the ArrayEnumerator (pass the mapped sequence as array) and implement IDictionaryEnumerator with members Key and Value to ArrayEnumerator.Current

nvm, not a good approach

the .net implementation is private HashtableEnumerator

enricosada · 2015-05-11T12:42:49Z

@forki ok got it, you have a pull request forki#2

add IDictionaryEnumerator

enricosada · 2015-05-11T15:03:48Z

src/fsharp/FSharp.Core/fslib-extra-pervasives.fs

@@ -70,6 +81,78 @@ module ExtraTopLevelOperators =
                    let key = RuntimeHelpers.StructBox(k)
                    if d.ContainsKey(key) then (r <- d.[key]; true) else false
                member s.Remove(k : 'Key) = (raise (NotSupportedException(SR.GetString(SR.thisValueCannotBeMutated))) : bool) 
+#if FX_NO_READONLY_COLLECTIONS


FX_ATLEAST_45 is not enough? IReadOnlyDictionary msdn

had to exclude portable

FSHARP_CORE_PORTABLE? because there are really a lot of defines already.
i mean FX_ATLEAST_45 && (!FSHARP_CORE_PORTABLE)

if FX_ATLEAST_45 && not FSHARP_CORE_PORTABLE

but why not explicitly exlude with ne flag. seems like a common pattern here

there are already lot of defines, and is going to be messier with netcore.
i dont think the best long term is one define per class, instead we should simplify the codebase and build permutations
Another factor is: if you undefine FX_NO_READONLY_COLLECTIONS, that's not enogh to compile if the class doesn't exists in the framework. Is not a pure feature flag (like WSDL define )
but that's my idea /cc @dsyme

Please use feature-based defines (FX_NO_READONLY_COLLECTIONS) rather than platform defines (FX_ATLEAST_45, FSHARP_CORE_PORTABLE). Granularity of "features" is up to you - merge related ones where possible, split where necessary,

thanks
Don

thx for reply @dsyme nice to know 😄

forki · 2015-05-12T06:54:27Z

@dsyme so I finish at 50a2e40

enricosada · 2015-05-12T08:36:01Z

i am pretty sure you need some unit tests

forki · 2015-05-12T08:39:02Z

yep. but I didn't find tests for the original dict function

2015-05-12 10:36 GMT+02:00 Enrico Sada notifications@github.com:

i am pretty sure you need some unit tests

—
Reply to this email directly or view it on GitHub
#436 (comment)
.

latkin · 2015-05-12T17:42:39Z

src/fsharp/FSharp.Core/fslib-extra-pervasives.fs

+                member s.ContainsKey(k) = d.ContainsKey(RuntimeHelpers.StructBox(k))
+                member s.TryGetValue(k,r) = 
+                    let key = RuntimeHelpers.StructBox(k)
+                    if d.ContainsKey(key) then (r <- d.[key]; true) else false


Can use d.TryGetValue here

type DictWrapper<'k,'v>(d : Dictionary<_,_>) = member __.TryGetValue(a : 'k, [<System.Runtime.InteropServices.Out>]b : 'v byref) = d.TryGetValue(a, &b)

Might as well fix above, too.

latkin · 2015-05-12T19:26:01Z

Looks good so far, just a few comments on properly filling out the various interfaces. Definitely needs tests.

I'd say this should be post-F# 4.0. Can we release it without changing the FSharp.Core version number? Technically the signature of dict returns just IDictionary<_,_>, so the public contract is not changing. But capabilities are clearly changed, so it's effectively the same as surface area change.

fix syncroot

latkin · 2015-08-04T18:05:04Z

This has been idle for over 2 months, closing out.

If remaining feedback can be addressed and tests added we'd be happy to including this in any upcoming release (doesn't change surface area so should be ok to add in an update).

dsyme · 2015-08-04T18:18:11Z

@forki would be good to have this in the pipe

forki · 2015-08-04T19:54:14Z

I stopped because everything in F# repo got stopped...
On Aug 4, 2015 8:18 PM, "Don Syme" notifications@github.com wrote:

@forki https://github.com/forki would be good to have this in the pipe

—
Reply to this email directly or view it on GitHub
#436 (comment)
.

forki · 2015-08-05T07:36:20Z

@latkin after closing this, what's your plan to get the feature in?

dsyme · 2015-08-05T12:22:00Z

@forki I think you can just reopen with the testing added. Closing was just spring cleaning, some work of mine got closed and I reopened with some progress.

latkin · 2015-08-06T20:48:29Z

@forki @dsyme yes, please just re-open when the known feedback is addressed, or open a new one if you prefer. Sorry to be "bad cop" this week, just trying to tidy up the PR and issue list.

forki · 2015-08-06T21:18:05Z

Actually it's very important to close things. Sorry for being a bit grumpy.
On Aug 6, 2015 10:48 PM, "Lincoln Atkinson" notifications@github.com
wrote:

@forki https://github.com/forki @dsyme https://github.com/dsyme yes,
please just re-open when the known feedback is addressed, or open a new one
if you prefer. Sorry to be "bad cop" this week, just trying to tidy up the
PR and issue list.

—
Reply to this email directly or view it on GitHub
#436 (comment)
.

Try to implement System.Collections.IDictionary for dict

dba8190

msftclas added the cla-already-signed label May 11, 2015

enricosada and others added 2 commits May 11, 2015 14:44

add IDictionaryEnumerator

0fcad7f

Merge pull request #2 from enricosada/pr_436_dict

e51598a

add IDictionaryEnumerator

forki force-pushed the dict branch 4 times, most recently from 05e662a to 945deac Compare May 11, 2015 14:09

forki changed the title ~~WIP: Try to implement System.Collections.IDictionary for dict~~ Implement System.Collections.IDictionary for dict May 11, 2015

forki force-pushed the dict branch from 945deac to 5dabc6d Compare May 11, 2015 14:58

enricosada reviewed May 11, 2015
View reviewed changes

Implement IReadOnlyDictionary<'Key, 'T> and IReadOnlyCollection for dict

50a2e40

forki force-pushed the dict branch from 5dabc6d to 50a2e40 Compare May 12, 2015 06:26

latkin reviewed May 12, 2015
View reviewed changes

enricosada and others added 2 commits May 13, 2015 10:17

fix syncroot

1ea9629

Merge pull request #3 from enricosada/pr_436_dict_syncroot

55aadca

fix syncroot

KevinRansom added the V.Next label May 19, 2015

latkin closed this Aug 4, 2015

forki mentioned this pull request Aug 5, 2015

Should F# lists implement ICollection? #570

Closed

latkin removed the V.Next label Aug 7, 2015

Implement System.Collections.IDictionary for dict #436

Implement System.Collections.IDictionary for dict #436

Uh oh!

Conversation

forki commented May 11, 2015

Uh oh!

enricosada commented May 11, 2015

Uh oh!

forki commented May 11, 2015

Uh oh!

enricosada commented May 11, 2015

Uh oh!

enricosada commented May 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forki commented May 12, 2015

Uh oh!

enricosada commented May 12, 2015

Uh oh!

forki commented May 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

latkin commented May 12, 2015

Uh oh!

latkin commented Aug 4, 2015

Uh oh!

dsyme commented Aug 4, 2015

Uh oh!

forki commented Aug 4, 2015

Uh oh!

forki commented Aug 5, 2015

Uh oh!

dsyme commented Aug 5, 2015

Uh oh!

latkin commented Aug 6, 2015

Uh oh!

forki commented Aug 6, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants