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

FMMultivalueLink inherits from SequenceableCollection #65

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

Gabriel-Darbord
Copy link
Member

@Gabriel-Darbord Gabriel-Darbord commented Dec 14, 2023

Instead of Collection.

Add new: to be compatible with the SequenceableCollection API.
Add setCollection: to make new: work with the intended pre-sizing mechanism.
Remove hash and first to ninth because they are defined in SequenceableCollection.

This might make FMMultivalueLink faster because of the optimized versions of select:thenCollect:&co, although I haven't tried it.

I'm also considering about removing the = method, which only differs from SequenceableCollection's by calling asOrderedCollection on the other collection, but that may be unnecessary (and thus slower for no reason).

	self == otherCollection ifTrue: [^ true].
	self species == otherCollection species ifFalse: [^ false].
	^ values hasEqualElements: otherCollection "=>" asOrderedCollection "<= unnecessary?"

…ection

Add `new:` to be compatible with the SequenceableCollection API
Add `setCollection:` to make `new:` work with the intended pre-sizing mechanism
Remove `hash` and `first` to `ninth` because they are defined in SequenceableCollection
@jecisc jecisc merged commit 48515e0 into development Dec 14, 2023
2 checks passed
@Gabriel-Darbord
Copy link
Member Author

The tests pass but this doesn't work as expected: we get errors when using basic Collection methods like collect:.

@jecisc
Copy link
Member

jecisc commented Dec 18, 2023

Do we revert?

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.

2 participants