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.

Sign up for GitHub

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

Jump to bottom

class library: fix issues with asPairs, asAssociations, asDict #3101

Merged
merged 1 commit into from
Aug 11, 2017
Merged

class library: fix issues with asPairs, asAssociations, asDict #3101

merged 1 commit into from
Aug 11, 2017

Conversation

miguel-negrao
Copy link
Member

I was getting strange results with asPairs and asAssociations and I think I found some bugs. Some elements were missing after doing asPairs or asAssociations.

When going from a dictionary to an array of associations the size should stay the same. the method in Collection was being called which was halving the size:

(
x = Dictionary.new;
100.do{ |i| x.put(i,i) };
x.asAssociations.size;
)
//got 64 should be at least 100

When going from an array of associations to an array of pairs the size should double, and the code in Collection was halving it.

(
x = Array.new;
100.do{ |i| x = x.add(i -> i) };
x.asPairs.size;
)
//got 64 should be at least 200

@miguel-negrao miguel-negrao added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: class library SC class library labels Aug 3, 2017
@miguel-negrao miguel-negrao requested a review from telephon August 3, 2017 20:53
@kisielk
Copy link
Contributor

kisielk commented Aug 3, 2017

asAssociations also fails for Set which is a superclass of Dictionary:

(
x = Set.new;
100.do{ |i| x.add(i) };
x.asAssociations.size;
)

results in:

ERROR: 'isAssociationArray' should have been implemented by Set.

@telephon
Copy link
Member

telephon commented Aug 3, 2017

yes, I also found them. They are fixed in #3020. Maybe you can take a look if those fixes there are sufficient? I'd like to avoid merge conflicts (well, if possible).

@miguel-negrao
Copy link
Member Author

Just from looking at it, your fix seems to do the same, and is more elegant so I say we use your fix.
The fix is unrelated with that PR though, I don't know if you want to wait until the whole PR is approved or just commit the fix in the meantime as separate commit/PR. In any case I understand if you don't want to mess with git and prefer to just wait until that PR is merged.

@telephon
Copy link
Member

telephon commented Aug 4, 2017

reading @LFSaw s comments on #3020, i think that one will take longer, because I cant fix it till September. So if 3.9 should happen before that, it would be better if you could just copy the solution there to a separate pull request that fixes the current problem (that would be really good)

@telephon
Copy link
Member

telephon commented Aug 4, 2017

@kisielk it is intended to fail for unordered collections, apart from dictionaries.

@miguel-negrao miguel-negrao force-pushed the fixAsPairs branch 2 times, most recently from 95b036e to 4b69e81 Compare August 4, 2017 10:12
@miguel-negrao
Copy link
Member Author

@telephon Ok, copied your fix. Also added some tests. Btw 'TestDictionary.run' fails strangely with ERROR: Message 'assert' not understood RECEIVER: Dictionary[ ]. Seems to have something with using a try block in the assert ?

@kisielk
Copy link
Contributor

kisielk commented Aug 4, 2017

@telephon I'm curious why that's the case? It could certainly work for sets by returning pairs of elements with themselves or associations to elements to themselves. I guess that's probably out of scope of this ticket though.

@telephon
Copy link
Member

telephon commented Aug 8, 2017

it could work for sets of associations, but not for sets of keys and values. Currently the definition of an array of associations is (trustingly) that the first element is an association. There is no first element for a set.

As you say, it is thinkable, but a little bit out of scope.

@telephon
Copy link
Member

telephon commented Aug 8, 2017

@miguel-negrao yes I noticed, i have no idea what causes it

@miguel-negrao
Copy link
Member Author

@telephon: @adcxyz has a fix for that already in another branch : 8dfc150

@miguel-negrao
Copy link
Member Author

Any reason not to merge this ?

@telephon
Copy link
Member

merge what? this #3101 pr?

@miguel-negrao
Copy link
Member Author

@telephon yes, this #3101 pr. It is an urgent fix, so I think it should be merged a.s.a.p..

@telephon
Copy link
Member

looks good

@miguel-negrao
Copy link
Member Author

Added newline.

@telephon telephon merged commit f207182 into supercollider:master Aug 11, 2017
@miguel-negrao
Copy link
Member Author

Build status appears as failed, it says 10 Tests failed, but I can't find which tests failed... anyone has any pointers ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@telephon telephon Awaiting requested review from telephon

Assignees
No one assigned
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: class library SC class library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@miguel-negrao @kisielk @telephon