Topic improve as event #2871

telephon · 2017-05-08T16:24:40Z

Key value pairs is the most straightforward interpretation. It matches
asDict, and has a complement asPairs.

I'd add the documentation when we have agreed on the correctness.

Dictionary classes are very different in functionality, so it is better to provide a standard way to specify what kind it should be.

Key value pairs is the most straightforward interpretation. It matches `asDict`, and has a complement `asPairs`.

nhthn · 2017-05-30T17:22:59Z

can you describe the changes in more detail?

ghost · 2017-05-30T18:32:58Z

i vote it up

telephon · 2017-05-30T18:36:58Z

yes, i can do this. Today i had an idea for a little improvement, I'll add that, too.

For uniformity, not only `asDict` should have a class argument, but also its complements. This commit adds them.

telephon · 2017-06-04T21:02:42Z

And here comes some explanation.

The best documentation of all this is in the Key Value Pairs helpfile. Somehow it doesn't turn up when I search for asPairs. It seems that the help doesn't do a full text search.

In short, this interface allows us to easily move between three very common data structures for parameters:

key-value-pairs (usually arrays, like [\freq, 600, \amp, 0.3])
collections of associations (usually arrays, like [\freq -> 600, \amp -> 0.3])
dictionaries (usually events, like (freq: 600, amp: 0.3))

each have advantages and disadvantages. e.g. (2) you can use very well for ordered pattern matching in rule systems, but for lookup (3) is better. (2) is used in synth arguments.

Dependent on what you want to do, it may matter what class the collection of dictionary is. E.g. strings shouldn't be used as keys in an IdentityDictionary if you need them for lookup. But for symbols, the more efficient lookup is IdentityDictionary.

This PR allows us to specify which class to use.

Also, it unifies the interface, so that you can expect the correct class as return vale in all cases.

Finally, it implements asEvent, as a common shortcut.

mossheim

Could you also implement the asEvent shortcut on Dictionary as well? I found myself wanting to use it while testing, and it makes sense to me to have it for completeness.

here's the test code I used in case anyone else wants to try:

(
a = [ 1, 2, 3, 4 ];

a.asAssociations; // creates an array of associations [ (1->2), (3->4) ]
a.asAssociations(Bag); // creates a Bag with the same associations

b = List[ 5, 6, 7, 8 ];
b.asAssociations(Array); // same effect as a.asAssociations above

a.asPairs; // returns self
a.asPairs(Dictionary); // returns a Dictionary [ (3->4), (1->2) ]
a.asPairs(Bag); // not very useful, but works

a.asDict({}, Dictionary); // creates a Dictionary with associations
a.asDict({}); // default: IdentityDictionary
a.asDict({}, Event); // creates an Event

b.asDictWith({}, Dictionary); // creates a Dictionary with associations
b.asDictWith({}); // default: IdentityDictionary
b.asDictWith({}, Event); // creates an Event

c = Dictionary[ 1->2, 3->4, 5->4 ];
c.asPairs; // an Array
c.asPairs(Bag); // a Bag, not very useful but it works
c.asDict({}, IdentityDictionary); // converts to IdentityDictionary
c.asDict({}, Event); // converts to Event
c.asDict({}, NodeMap); // NodeMap is also a type of Dictionary
c.asDict({}, Event)
a.asDict(nil, Event);
a.asEvent;

b.asDict(nil, Event);
b.asEvent;
)

mossheim · 2017-06-08T17:05:37Z

SCClassLibrary/Common/Collections/Dictionary.sc

@@ -296,12 +296,17 @@ Dictionary : Set {

 	isAssociationArray { ^false }

-	asPairs {
-		^this.getPairs
+	asPairs { |class|


class here needs documentation.

mossheim · 2017-06-08T17:05:52Z

SCClassLibrary/Common/Collections/Dictionary.sc

 	}

-	asDict {
-		^this
+	asDict { arg mergeFunc, class;


Would you mind changing this arg to pipes? (Or change pipes to args in asPairs above, that seems to be file-consistent)

I'd only do this for the whole file. I'd prefer to keep the surrounding style, and change it some time in the future, all at once in a separate PR.

resolved (for now)

mossheim · 2017-06-08T17:06:59Z

HelpSource/Classes/Dictionary.schelp

+method::asDict
+If no arguments are passed, return itself.  This is part of the link::Reference/Key-Value-Pairs:: interface.
+argument::mergeFunc
+This argument is not used, but exists to make the method compatible.


can we add "compatible with Collection.asDict" (with a link to that method)? This may be confusing out of context.

mossheim · 2017-06-08T17:07:32Z

SCClassLibrary/Common/Collections/SequenceableCollection.sc

@@ -929,6 +929,7 @@ SequenceableCollection : Collection {

 	asPoint { ^Point(this[0] ? 0, this[1] ? 0) }
 	asRect { ^Rect(this[0] ? 0, this[1] ? 0, this[2] ? 0, this[3] ? 0) }
+	asEvent { |mergeFunc| ^this.asDict(mergeFunc, Event) }


This needs documentation.

mossheim · 2017-06-08T17:08:17Z

SCClassLibrary/Common/Collections/Collection.sc

 		this.pairsDo { |key, val| res.add(key -> val) }
 		^res
 	}

-	asPairs {
+	asPairs { |class|


IMO this method needs documentation, now that it has an additional parameter that can be explained.

mossheim · 2017-06-08T17:11:55Z

HelpSource/Reference/Key-Value-Pairs.schelp

@@ -44,7 +44,39 @@ CODE::
 [\freq -> 452, \amp -> 0.2].asDict
 (freq: 452, amp: 0.2).asDict


This example fails for me, I think because Dictionary:-asDict should choose IdentityDictionary as its default class like Collection:-asDict.

seems resolved

mossheim · 2017-06-08T17:13:31Z

HelpSource/Classes/Dictionary.schelp

 ::

+method::asKeyValuePairs
+See asPairs.


I would suggest adding a link here, I think, in case these methods ever get moved around and their definitions move apart.

This makes it work for all collections, also for Dictionaries.

telephon · 2017-06-09T16:04:18Z

… here you are. Thanks for the review.

mossheim

Thanks for the quick response! This looks good to me now. Very slick.

This requires the method assertEvery in a separate pull request made for the UnitTesting quark.

mossheim · 2017-06-09T21:09:36Z

Oof.. I'm really not a fan of adding an assertEvery method. IMHO it would only seem to make it easier to combine tests together that should really be separate units. That, and a few other concerns:

the tests are in TestEvent, but they test methods on multiple classes (dictionary and array)
the failure messages are uninformative, specifically because multiple functions which test different behaviors are being passed through the same interface
the high test volume is mostly redundant: if each conversion were tested separately, there would be no need to test that cyclic chained conversions like .asPairs.asDict.asAssociations.asPairs are correct, because it would follow naturally once the individual conversions are shown to be correct.

Maybe we should have a conversation about this on the mailing list/dev call... I've only brought it up a few times but it seems like there is a significant difference among testing philosophies on this project and it might benefit from some discussion.

telephon · 2017-06-09T21:58:23Z

I wrote assertEvery because recently you had (correctly) requested that in multiple tests, it should be possible to know which one has failed. I don't know if this is just me, but the every method to me seems just so much a natural extension of the if method that I use it all the time when I test several things.

The example test I did (independent of whether you agree on its value) shows that it makes it easy to combinatorially test large sets. Because a failure will post both the failed test function's source code and its index in the array, you can easily find which test has failed.

We have no place for interface tests, so I put them in Event. I can move the tests elsewhere. Suggestions?
(see above) - is there anything you miss?
ok, the fun fair is a bit luxurious. I'll try and reduce it to the necessary.

mossheim · 2017-06-09T22:53:09Z

Oh! I missed that source code is also posted on failure, that changes things. Most other languages don't have this much introspection (I'm thinking of the effort that goes into stringifying source code / test names in the boost.test lib) so I'm trying to hold back from judging this idea by the APIs provided by other test libs.

We have no place for interface tests, so I put them in Event. I can move the tests elsewhere. Suggestions?

Sorry, I'm not sure what distinguishes this as an "interface test", or why that would need special treatment. These are after all just methods sent to class instances. What I'm picking up on is that there some benefit to grouping these conversion methods in the same suite? How about a suite named specifically for that purpose, TestInterCollectionConversion? (working title :P)

(see above) - is there anything you miss?

No actually, I didn't fully understand what was going on at first, but now it makes more sense.

The only thing I am still concerned about is that even though it's true that this "makes it easy to combinatorially test large sets," it has the potential to obfuscate test code since there are now multiple layers of indirection. Each layer makes it more difficult to verify at a glance that the tests are testing the right things. Since test code is inherently high-volume (and this case is no exception), and since test code should be the first guard against bugs, I consider that problematic.

Changes: -move key-value-pairs tests to separate test class -use asserEquals and explicit iteration - add newFrom tests for collection conversion

telephon · 2017-06-10T08:51:35Z

ok, please take a look if that is better.

mossheim

I just really don't understand this testing approach, sorry... Why chain together 2-3 function calls? Why group three separately labeled sections in one test?

This one test (test_keyValuePairs) is now testing many different behaviors. It's more a matter of consistency than anything--is the "unit test" under consideration here a single method, or a single assert within a method? Is the "unit" one function call, or a single property of a group of function calls?

It seems to me that there are obvious benefits to preferring the former in both cases: modularity, clarity, self-documenting code, reduced chance of bugs, and conformance to existing testing patterns in this and other languages. For the latter cases, I don't see a strong benefit other than a reduction in lines of code.

mossheim · 2017-06-10T14:33:36Z

testsuite/classlibrary/TestConversions.sc

@@ -0,0 +1,62 @@
+TestConversions : UnitTest {


"Conversions" could cover a lot of methods, not all of which should be grouped together IMO. Can we rename this TestCollectionConversions? I think Collections are a good functionally contained unit to test on.

mossheim · 2017-06-10T14:36:48Z

testsuite/classlibrary/TestConversions.sc

+	}
+
+
+}


Missing EOF newline

newline still needed.

telephon · 2017-06-10T17:23:05Z

(design decision waiting on method table size outcome)

mossheim · 2017-06-14T03:11:07Z

https://github.com/brianlheim/sclang-method-table-size/

See also my email to sc-dev. I say we are safe to proceed with freely adding methods; even if we had two test methods for every method in SC it wouldn't make much of an impact. The test suite doesn't ship with the binaries, so most users are unaffected anyway. What do you think @telephon ?

telephon · 2017-06-16T10:27:09Z

@brianlheim better like this?

mossheim

Works for me, thanks!

mossheim · 2017-06-16T13:41:32Z

@snappizz or someone else, want to take a look at this quick? I'm too tired to trust myself with merging this right now. :)

telephon · 2017-06-16T13:44:46Z

@LFSaw maybe, as well?

LFSaw

I looked at the code and think it is in the state of merging.
Also, all comments by @brianlheim are resolved apart from the missing trailing newline in TestConversion.sc

LFSaw · 2017-06-29T08:20:57Z

HelpSource/Reference/Key-Value-Pairs.schelp

@@ -44,7 +44,39 @@ CODE::
 [\freq -> 452, \amp -> 0.2].asDict
 (freq: 452, amp: 0.2).asDict


seems resolved

LFSaw · 2017-06-29T08:21:10Z

SCClassLibrary/Common/Collections/Collection.sc

 		this.pairsDo { |key, val| res.add(key -> val) }
 		^res
 	}

-	asPairs {
+	asPairs { |class|


LFSaw · 2017-06-29T08:21:21Z

SCClassLibrary/Common/Collections/Dictionary.sc

@@ -296,12 +296,17 @@ Dictionary : Set {

 	isAssociationArray { ^false }

-	asPairs {
-		^this.getPairs
+	asPairs { |class|


LFSaw · 2017-06-29T08:21:29Z

SCClassLibrary/Common/Collections/Dictionary.sc

 	}

-	asDict {
-		^this
+	asDict { arg mergeFunc, class;


resolved (for now)

LFSaw · 2017-06-29T08:21:44Z

testsuite/classlibrary/TestConversions.sc

+	}
+
+
+}


newline still needed.

LFSaw · 2017-06-29T08:25:36Z

I can merge as soon as the newline appears :)

telephon added 2 commits May 8, 2017 16:06

class library: asDict allow specify class

469915b

Dictionary classes are very different in functionality, so it is better to provide a standard way to specify what kind it should be.

class library: asEvent for SequenceableCollection

fec2b80

Key value pairs is the most straightforward interpretation. It matches `asDict`, and has a complement `asPairs`.

nhthn added the comp: class library SC class library label May 8, 2017

nhthn added this to the 3.9 milestone May 8, 2017

mossheim added the enhancement label May 8, 2017

telephon added 3 commits May 18, 2017 09:59

class library: asDict should also take a class argument for Dictionary

22c5cb9

help: documentation for asDict

6e93113

help: document asPairs and asAssociations

2000e4d

telephon added 2 commits May 31, 2017 19:55

class library: asPairs and asAssociations pass class

1e1077e

For uniformity, not only `asDict` should have a class argument, but also its complements. This commit adds them.

class library: asPairs takes class argument

1d86e12

mossheim suggested changes Jun 8, 2017

View reviewed changes

telephon added 3 commits June 9, 2017 17:47

class library: move asEvent to collection

36cce07

This makes it work for all collections, also for Dictionaries.

help: update asPairs documentation

8712021

class library: dictionary asDict default class

fa17b85

mossheim approved these changes Jun 9, 2017

View reviewed changes

test: key-value-pair test

da09b79

This requires the method assertEvery in a separate pull request made for the UnitTesting quark.

test: fix test test

e056b84

test: move key-value-pairs tests to separate test class

4db78b5

Changes: -move key-value-pairs tests to separate test class -use asserEquals and explicit iteration - add newFrom tests for collection conversion

test: post function that failed or succeeded.

0d4c0b8

mossheim reviewed Jun 10, 2017

View reviewed changes

telephon added the Work In Progress (WIP) - don't merge yet label Jun 10, 2017

test: factor methods into smaller parts

3fb748b

mossheim approved these changes Jun 16, 2017

View reviewed changes

telephon removed the Work In Progress (WIP) - don't merge yet label Jun 16, 2017

LFSaw reviewed Jun 29, 2017

View reviewed changes

add an extra newline

8bc81e1

mossheim merged commit c3e6532 into supercollider:master Jun 29, 2017

		@@ -44,7 +44,39 @@ CODE::
		[\freq -> 452, \amp -> 0.2].asDict
		(freq: 452, amp: 0.2).asDict

Topic improve as event #2871

Topic improve as event #2871

Conversation

telephon commented May 8, 2017

nhthn commented May 30, 2017

ghost commented May 30, 2017

telephon commented May 30, 2017

telephon commented Jun 4, 2017

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telephon commented Jun 9, 2017

mossheim left a comment

Choose a reason for hiding this comment

mossheim commented Jun 9, 2017

telephon commented Jun 9, 2017

mossheim commented Jun 9, 2017 • edited Loading

telephon commented Jun 10, 2017

mossheim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telephon commented Jun 10, 2017

mossheim commented Jun 14, 2017

telephon commented Jun 16, 2017

mossheim left a comment

Choose a reason for hiding this comment

mossheim commented Jun 16, 2017

telephon commented Jun 16, 2017

LFSaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LFSaw commented Jun 29, 2017

mossheim commented Jun 9, 2017 •

edited

Loading

mossheim left a comment •

edited

Loading