The Inbox: CollectionsTests-cmm.312.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

The Inbox: CollectionsTests-cmm.312.mcz

commits-2
Chris Muller uploaded a new version of CollectionsTests to project The Inbox:
http://source.squeak.org/inbox/CollectionsTests-cmm.312.mcz

==================== Summary ====================

Name: CollectionsTests-cmm.312
Author: cmm
Time: 4 June 2019, 3:32:07.434178 pm
UUID: b63a7b5c-085b-4479-bc8d-8910d2afeeaf
Ancestors: CollectionsTests-mt.311

Additional test for #joinSeparatedBy: describes the expected behavior for unordered collections.

=============== Diff against CollectionsTests-mt.311 ===============

Item was added:
+ ----- Method: CollectionTest>>testJoin (in category 'tests') -----
+ testJoin
+ | joined |
+ self assert: #(a b c d e) join = 'abcde'.
+ self assert: (#(a b c) joinSeparatedBy: '|') = 'a|b|c'.
+ joined := (#(a b c) asSet) joinSeparatedBy: '|'.
+ self assert: (#(2 4) allSatisfy: [ : index | (joined at: index) = $| ]).
+ self assert: (#(1 3 5) allSatisfy: [ : index | 'abc' includes: (joined at: index) ])!

Item was removed:
- ----- Method: SequenceableCollectionTest>>testJoin (in category 'tests - converting') -----
- testJoin
-
- self assert: #(a b c d e) join = 'abcde'.
- self assert: (#(a b c) joinSeparatedBy: '|') = 'a|b|c'.!


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: CollectionsTests-cmm.312.mcz

Nicolas Cellier
Note that joined='a|a|a' would pass the test. A longer joined too.

Le mar. 4 juin 2019 à 22:32, <[hidden email]> a écrit :
Chris Muller uploaded a new version of CollectionsTests to project The Inbox:
http://source.squeak.org/inbox/CollectionsTests-cmm.312.mcz

==================== Summary ====================

Name: CollectionsTests-cmm.312
Author: cmm
Time: 4 June 2019, 3:32:07.434178 pm
UUID: b63a7b5c-085b-4479-bc8d-8910d2afeeaf
Ancestors: CollectionsTests-mt.311

Additional test for #joinSeparatedBy: describes the expected behavior for unordered collections.

=============== Diff against CollectionsTests-mt.311 ===============

Item was added:
+ ----- Method: CollectionTest>>testJoin (in category 'tests') -----
+ testJoin
+       | joined |
+       self assert: #(a b c d e) join = 'abcde'.
+       self assert: (#(a b c) joinSeparatedBy: '|') = 'a|b|c'.
+       joined := (#(a b c) asSet) joinSeparatedBy: '|'.
+       self assert: (#(2 4) allSatisfy: [ : index | (joined at: index) = $| ]).
+       self assert: (#(1 3 5) allSatisfy: [ : index | 'abc' includes: (joined at: index) ])!

Item was removed:
- ----- Method: SequenceableCollectionTest>>testJoin (in category 'tests - converting') -----
- testJoin
-
-       self assert: #(a b c d e) join = 'abcde'.
-       self assert: (#(a b c) joinSeparatedBy: '|') = 'a|b|c'.!




Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: CollectionsTests-cmm.312.mcz

Chris Muller-3
Okay.  Fixed in CollectionsTests-cmm.313.mcz.

On Tue, Jun 4, 2019 at 4:13 PM Nicolas Cellier
<[hidden email]> wrote:

>
> Note that joined='a|a|a' would pass the test. A longer joined too.
>
> Le mar. 4 juin 2019 à 22:32, <[hidden email]> a écrit :
>>
>> Chris Muller uploaded a new version of CollectionsTests to project The Inbox:
>> http://source.squeak.org/inbox/CollectionsTests-cmm.312.mcz
>>
>> ==================== Summary ====================
>>
>> Name: CollectionsTests-cmm.312
>> Author: cmm
>> Time: 4 June 2019, 3:32:07.434178 pm
>> UUID: b63a7b5c-085b-4479-bc8d-8910d2afeeaf
>> Ancestors: CollectionsTests-mt.311
>>
>> Additional test for #joinSeparatedBy: describes the expected behavior for unordered collections.
>>
>> =============== Diff against CollectionsTests-mt.311 ===============
>>
>> Item was added:
>> + ----- Method: CollectionTest>>testJoin (in category 'tests') -----
>> + testJoin
>> +       | joined |
>> +       self assert: #(a b c d e) join = 'abcde'.
>> +       self assert: (#(a b c) joinSeparatedBy: '|') = 'a|b|c'.
>> +       joined := (#(a b c) asSet) joinSeparatedBy: '|'.
>> +       self assert: (#(2 4) allSatisfy: [ : index | (joined at: index) = $| ]).
>> +       self assert: (#(1 3 5) allSatisfy: [ : index | 'abc' includes: (joined at: index) ])!
>>
>> Item was removed:
>> - ----- Method: SequenceableCollectionTest>>testJoin (in category 'tests - converting') -----
>> - testJoin
>> -
>> -       self assert: #(a b c d e) join = 'abcde'.
>> -       self assert: (#(a b c) joinSeparatedBy: '|') = 'a|b|c'.!
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: CollectionsTests-cmm.312.mcz

Nicolas Cellier
Hi Chris,
I did not mean we need a longer joined example.
I did mean that the test omitted to check joined length, so an implementation returning 'a|b|c|' would pass...
Also each of abc should be found in joined rather than each of joined found in abc, otherwise 'a|a|a' would pass... I fail to see how you changed that in next version, but it's late, so i have an excuse ;)
Also, for the bag case, i would expect to find the 5 z in joined,  and i do not see it in the new version of the test.
IMO, the unordered case is going to need a separate method, because it already takes too much place relatively to more usual sequenceable case.
The difficulty to write these tests, and relative length of sequenceable/unordered cases speak by themself: it perfectly illustrate non obvioussness of the feature, that's why i asked.
So yes, we can easily implement the feature, but should we? IMO, we ain't gonna need it, or so rarely...
To make more sense out of the test, we could select joined at odd indices, sorted, and check that it equals original collection sorted (because it's  like testing sequenceable shuffled join:)...


Le mar. 4 juin 2019 à 23:55, Chris Muller <[hidden email]> a écrit :
Okay.  Fixed in CollectionsTests-cmm.313.mcz.

On Tue, Jun 4, 2019 at 4:13 PM Nicolas Cellier
<[hidden email]> wrote:
>
> Note that joined='a|a|a' would pass the test. A longer joined too.
>
> Le mar. 4 juin 2019 à 22:32, <[hidden email]> a écrit :
>>
>> Chris Muller uploaded a new version of CollectionsTests to project The Inbox:
>> http://source.squeak.org/inbox/CollectionsTests-cmm.312.mcz
>>
>> ==================== Summary ====================
>>
>> Name: CollectionsTests-cmm.312
>> Author: cmm
>> Time: 4 June 2019, 3:32:07.434178 pm
>> UUID: b63a7b5c-085b-4479-bc8d-8910d2afeeaf
>> Ancestors: CollectionsTests-mt.311
>>
>> Additional test for #joinSeparatedBy: describes the expected behavior for unordered collections.
>>
>> =============== Diff against CollectionsTests-mt.311 ===============
>>
>> Item was added:
>> + ----- Method: CollectionTest>>testJoin (in category 'tests') -----
>> + testJoin
>> +       | joined |
>> +       self assert: #(a b c d e) join = 'abcde'.
>> +       self assert: (#(a b c) joinSeparatedBy: '|') = 'a|b|c'.
>> +       joined := (#(a b c) asSet) joinSeparatedBy: '|'.
>> +       self assert: (#(2 4) allSatisfy: [ : index | (joined at: index) = $| ]).
>> +       self assert: (#(1 3 5) allSatisfy: [ : index | 'abc' includes: (joined at: index) ])!
>>
>> Item was removed:
>> - ----- Method: SequenceableCollectionTest>>testJoin (in category 'tests - converting') -----
>> - testJoin
>> -
>> -       self assert: #(a b c d e) join = 'abcde'.
>> -       self assert: (#(a b c) joinSeparatedBy: '|') = 'a|b|c'.!
>>
>>
>



Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: CollectionsTests-cmm.312.mcz

Nicolas Cellier


Le mer. 5 juin 2019 à 00:29, Nicolas Cellier <[hidden email]> a écrit :
Hi Chris,
I did not mean we need a longer joined example.
I did mean that the test omitted to check joined length, so an implementation returning 'a|b|c|' would pass...
Also each of abc should be found in joined rather than each of joined found in abc, otherwise 'a|a|a' would pass... I fail to see how you changed that in next version, but it's late, so i have an excuse ;)
Also, for the bag case, i would expect to find the 5 z in joined,  and i do not see it in the new version of the test.
IMO, the unordered case is going to need a separate method, because it already takes too much place relatively to more usual sequenceable case.
The difficulty to write these tests, and relative length of sequenceable/unordered cases speak by themself: it perfectly illustrate non obvioussness of the feature, that's why i asked.
So yes, we can easily implement the feature, but should we? IMO, we ain't gonna need it, or so rarely...
To make more sense out of the test, we could select joined at odd indices, sorted, and check that it equals original collection sorted (because it's  like testing sequenceable shuffled join:)...

Or better, joined substrings sorted equals: (original collect: asString) sorted.

Le mar. 4 juin 2019 à 23:55, Chris Muller <[hidden email]> a écrit :
Okay.  Fixed in CollectionsTests-cmm.313.mcz.

On Tue, Jun 4, 2019 at 4:13 PM Nicolas Cellier
<[hidden email]> wrote:
>
> Note that joined='a|a|a' would pass the test. A longer joined too.
>
> Le mar. 4 juin 2019 à 22:32, <[hidden email]> a écrit :
>>
>> Chris Muller uploaded a new version of CollectionsTests to project The Inbox:
>> http://source.squeak.org/inbox/CollectionsTests-cmm.312.mcz
>>
>> ==================== Summary ====================
>>
>> Name: CollectionsTests-cmm.312
>> Author: cmm
>> Time: 4 June 2019, 3:32:07.434178 pm
>> UUID: b63a7b5c-085b-4479-bc8d-8910d2afeeaf
>> Ancestors: CollectionsTests-mt.311
>>
>> Additional test for #joinSeparatedBy: describes the expected behavior for unordered collections.
>>
>> =============== Diff against CollectionsTests-mt.311 ===============
>>
>> Item was added:
>> + ----- Method: CollectionTest>>testJoin (in category 'tests') -----
>> + testJoin
>> +       | joined |
>> +       self assert: #(a b c d e) join = 'abcde'.
>> +       self assert: (#(a b c) joinSeparatedBy: '|') = 'a|b|c'.
>> +       joined := (#(a b c) asSet) joinSeparatedBy: '|'.
>> +       self assert: (#(2 4) allSatisfy: [ : index | (joined at: index) = $| ]).
>> +       self assert: (#(1 3 5) allSatisfy: [ : index | 'abc' includes: (joined at: index) ])!
>>
>> Item was removed:
>> - ----- Method: SequenceableCollectionTest>>testJoin (in category 'tests - converting') -----
>> - testJoin
>> -
>> -       self assert: #(a b c d e) join = 'abcde'.
>> -       self assert: (#(a b c) joinSeparatedBy: '|') = 'a|b|c'.!
>>
>>
>



Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: CollectionsTests-cmm.312.mcz

Chris Muller-4
In reply to this post by Nicolas Cellier
Hi Nicolas,



I did mean that the test omitted to check joined length, so an implementation returning 'a|b|c|' would pass...

The original test didn't test that either, but I went ahead and added it into CollectionsTests-cmm.315.
 
Also, for the bag case, i would expect to find the 5 z in joined,  and i do not see it in the new version of the test.

CollectionsTests-cmm.315 now covers that.  All green.
 
IMO, the unordered case is going to need a separate method, because it already takes too much place relatively to more usual sequenceable case.

With this new test, I hope you're convinced otherwise now.
 
The difficulty to write these tests, and relative length of sequenceable/unordered cases speak by themself: it perfectly illustrate non obvioussness of the feature, that's why i asked.

They were _not_ difficult to write at all.  Nicolas, I wrote and improved them in near real-time to your suggestions!  Now they're more robust than they were before.   Your #sorted comparison was genius.

If there's any confusion left, Marcel's original comment says all there is to say:

       "Returns a string, which is a concatenation of each element's string representation separated by another string."

"Each element's".  That's it.  It says _nothing_ about sequence.  It also basically duplicates Goran's original Collection>>#asStringOn:delimiter:, which has never been questioned.  On SequenceableCollection is questionable, IMO.
 
So yes, we can easily implement the feature, but should we? IMO, we ain't gonna need it, or so rarely...

We absolutely should.  I'm going to want it when I get to converting Magma <--> GraphQL query's at performance.
 
 - Chris


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: CollectionsTests-cmm.312.mcz

Chris Muller-3
On Tue, Jun 4, 2019 at 9:55 PM Chris Muller <[hidden email]> wrote:
>
> Hi Nicolas,
>
>>
>>
>> I did mean that the test omitted to check joined length, so an implementation returning 'a|b|c|' would pass...
>
>
> The original test didn't test that either, but I went ahead and added it into CollectionsTests-cmm.315.

Err, I guess it did, implicitly, as does the new one.