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

Improve RandomAccessList API with some functions from PersistentVector #54

Merged
merged 3 commits into from
May 25, 2016

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Feb 10, 2016

These are the easy functions, which aren't dependent on order of insertion. Functions like append and windowSeq will be harder, so wait until later to add those. But take the low-hanging fruit now.

Note that I have not yet written unit tests for these. These functions are straightforward, and I copied the implementations from PersistentVector and just changed the type signatures (and parameter names), so I doubt that there are any errors. So feel free to merge this now if you want; but if you want to hold off on merging this PR until I can write unit tests for these new functions, I can do that within the next couple of days.

These are the easy functions, which aren't dependent on order of
insertion. Functions like append and windowSeq will be harder, so wait
until later to add those. But take the low-hanging fruit now.
@forki
Copy link
Member

forki commented Feb 10, 2016

do we need tests for this?

@rmunn
Copy link
Contributor Author

rmunn commented Feb 10, 2016

Honestly, having looked at how extensive the test suite is, I'd feel better if anything I added also had tests, so I plan to write unit tests anyway whether or not this commit is merged before the tests are ready. But given the simplicity of these functions and how hard they would be to get wrong, I figured might as well create the PR now.

(I plan to try to write an implementation of append and windowSeq, too, but those will definitely need tests, as it would be easy to get it wrong and accidentally append a reversed version of the list you were trying to append. So those are going to wait until I have a bit more time; I'm running out of open-source coding time for today.)

@forki
Copy link
Member

forki commented Feb 10, 2016

I think most tests can be copied over from vector. But we should do this.

@rmunn
Copy link
Contributor Author

rmunn commented Feb 15, 2016

I have tests written now, and I'm about ready to update this PR with my changes. But there's one question of design I want to talk about before I update it: the design of the windowSeq function for RandomAccessLists. windowSeq n splits a list or vector into parts of size n, with a single part that may contain fewer than n items. The question is, should that maybe-smaller part be on the end or the start of the split list?

PersistentVectors conj on the end of the vector, so it's natural for their windowSeq function to have the smaller part be on the end. In other words, given a PersistentVector equivalent to [1;2;3;4;5;6], calling windowSeq 4 on it will yield the equivalent of [ [1;2;3;4]; [5;6] ].

But if we have a RandomAccessList equivalent to [1;2;3;4;5;6] and call windowSeq 4 on it, which one would most people expect as the result?

Smaller list first: [ [1;2] [3;4;5;6] ]

Smaller list last: [ [1;2;3;4] [5;6] ]

The code I've written so far follows smaller list first semantics for windowSeq, which fits its "mirror of PersistentVector" nature. But I suspect that that might violate the Principle of Least Surprise, and that other people might expect windowSeq to follow smaller list last semantics for RandomAccessLists, just as it does for PersistentVectors.

So, for you, which of those two options would be the most surprising? Given that RandomAccessList uses cons and prepends to itself, which of those two choices would you expect RandomAccessList.windowSeq to return?

Note: windowSeq semantics are to put the short list, if there is one,
first (the mirror of how PersistentVector.windowSeq works).
@rmunn
Copy link
Contributor Author

rmunn commented Feb 15, 2016

... Huh. I have F# 4 on my computer, and it builds fine there, with all unit tests passing. But since the Travis buildbot has F# 3.1, it won't build -- because I used Seq.foldBack to implement RandomAccessList.windowSeq. And foldBack was only added to seqs in F# 4.

I assume we still need to maintain 3.1 compatibility, right? I'll refactor RandomAccessList.windowSeq to use Seq.fold instead.

@forki
Copy link
Member

forki commented May 25, 2016

I think depending on FSharp.Core 4 is fine. Will take a look

@forki forki merged commit 8c896da into fsprojects:master May 25, 2016
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.

None yet

2 participants