#33, #34: *[.NonEmpty].init and .replicate, basic Numbers members #66
Conversation
…d some basic Numbers members
| static member op_Implicit (t:NaturalInt) = t.Value | ||
|
|
||
| member this.Increment = let (NaturalInt t) = this in PositiveInt (t + 1) | ||
| static member increment (t:NaturalInt) = t.Increment |
There was a problem hiding this comment.
Some of the .increment, .decrement, and .opposite methods are potentially subject to integer overflow, resulting in a Numbers type that doesn't match its assumption (like PositiveInt -2147483648).
The exceptions are PositiveInt.decrement and PositiveInt.opposite.
There was a problem hiding this comment.
Okay. I'm not terribly worried about overflow. I don't think that's something we could safely handle at runtime anyway.
|
|
||
| [<Struct>] | ||
| type NegativeInt = private NegativeInt of int | ||
| static member zero = NaturalInt 0 |
There was a problem hiding this comment.
In general, members should be PascalCase according to the F# style guide.
This is especially important for Zero. In the FSI, I can type:
> let inline f xs = List.sum xs;;
val inline f :
xs: ^a list -> ^a
when ^a : (static member ( + ) : ^a * ^a -> ^a) and
^a : (static member get_Zero : -> ^a)so if you capitalize this Zero, you should be able to sum a list of NaturalInts (and we might want a test to verify that), but with the lower-case zero, it wouldn't satisfy the standard Zero property constraint.
To be in line with the style guide, we should either move the static members to a module (with the same name as the type), or use Pascal case. Or both (would we ever want to use the static ones from C# for some reason?) But for Zero, we should definitely stick with a Pascal case static member so that we can work with functions like sum
There was a problem hiding this comment.
oh, well, I guess we'd have to define the (+) operator as a static member as well, which is probably a pretty good idea anyway. I wouldn't want (-), since you wouldn't be able to write it as (-) : NaturalInt -> NaturalInt -> NaturalInt
| static member op_Implicit (t:NaturalInt) = t.Value | ||
|
|
||
| member this.Increment = let (NaturalInt t) = this in PositiveInt (t + 1) | ||
| static member increment (t:NaturalInt) = t.Increment |
There was a problem hiding this comment.
Okay. I'm not terribly worried about overflow. I don't think that's something we could safely handle at runtime anyway.
| alwaysProduceSameOutputForSeq2 Seq.take' Seq.take | ||
| alwaysProduceSameOutputForSeq2 Seq.windowed' Seq.windowed | ||
|
|
||
| module SafeByType = |
There was a problem hiding this comment.
I'm wondering if this should be in it's own file. It's not unique to seqs anymore, right? (I'm just noticing that you use this from StringSpec.fs)
| /// <summary> | ||
| /// Returns the first element of the sequence. | ||
| /// Returns a SeqIsEmpty error if <c>xs</c> has no elements. | ||
| /// </summary> |
There was a problem hiding this comment.
Maybe this would be a separate issue, but would we want to consider something like an initLenient and replicateLenient that would just return an empty Seq if the given input is negative? Maybe not worthwhile, since it's not that much work to just do:
match count with
| Natural i -> Seq.init i ...
| NonNatural _ -> seq []| /// Generates a new sequence which, when iterated, will return successive elements by calling the given function. | ||
| /// Same as <c>Seq.initInfinite</c>, but provides NaturalInt indices to <c>initializer</c>. | ||
| /// </summary> | ||
| let initInfinitely initializer = |
There was a problem hiding this comment.
I'm not sure about this function. I prefer anything that would create an infinite sequence to be in the InfiniteSeq module. If you know that the sequence is infinite, you probably want that represented by the type system. Is there a function in InfiniteSeq that takes NaturalInt indices? If not, you should probably move this into the InfiniteSeq module (though maybe we should have a different name). If we do, then we could probably just rely on that function and get rid of this.
| open SafetyFirst.Numbers | ||
|
|
||
| let check prop = | ||
| Arb.register<Generators.Numbers>() |> ignore |
There was a problem hiding this comment.
I think we should use Prop.forAll here, which lets you specify an Arb instance to use instead of having to statically register it. See https://fscheck.github.io/FsCheck/Properties.html#Quantified-Properties. I realize that you probably only need one statically registered instance of Generators.Numbers, but statically registered Arb instances get really hard to keep track of compared to just passing the Arb instance you need in wherever you need it.
| /// generated. | ||
| /// </summary> | ||
| let init transform = InfiniteSeq (Seq.initInfinite transform) | ||
| let init transform = InfiniteSeq (Seq.initInfinite (transform << (NaturalInt.verify >> Option.unless "F# core assumption failed: Seq.initInfinite called an initializer with a negative index."))) |
There was a problem hiding this comment.
This is a breaking change, and I'm not ready to introduce a breaking change for this.
We could create a new function (if we can come up with a different name) that has this signature without it being a breaking change.
| let repeat x = | ||
| let rec s = cons x (delayed (fun () -> s)) in s | ||
|
|
||
| let initInfinitely (initializer:NaturalInt -> 'a) = |
There was a problem hiding this comment.
For these functions that are no longer straight from LazyList (if we need to keep them once FSharpx.Collections 2.0 is released) can we mark them as such with comments so that we know once we try deleting this type?
Also, will these be able to be deleted if the type disappears? Do they rely on private internals of this type?
|
|
||
| let replicateN count initial = | ||
| let rec s n () = match n with | ZeroNatural -> CellEmpty | PositiveNatural count -> consc initial (lzy (s count.Decrement)) | ||
| lzy (s count) |
There was a problem hiding this comment.
Can you explain these changes, since I never really bothered reading through all the LazyList source? I'm also interested in what our strategy is once FSharpx.Collections is updated to use netstandard2.0 and we end up wanting to delete this file.
|
|
||
| let replicateInfinitely initial = | ||
| let rec s () = consc initial (lzy s) | ||
| lzy s |
There was a problem hiding this comment.
Is this the same thing as the old repeat function? Why the change in implementation?
2 participants
I set out to do #33, and #34 came along for the ride.
Added init and replicate to LazyList/FSeq, Seq, List, and Array.
Added Numbers.value, .op_implicit, and lots of conversions. Also, Numbers FsCheck generators.