Correct Observable.filteri bug and optimise Observable.mapi #72
Conversation
Observable.filteri previously only took a predicate of type 'T -> bool and behaved identically to Observable.filter. The corrected version takes a predicate of type int -> 'T -> bool, matching the convention set by the F# standard library.
Previously, Observable.mapi was defined using Observable.scan and Observable.map, resulting in an unnecessary performance loss. The new definition is simpler and directly calls the correct overload of the Reactive extensions Observable.Select method.
| @@ -560,7 +560,7 @@ module Observable = | |||
| /// Filters the observable elements of a sequence based on a predicate by | |||
| /// incorporating the element's index | |||
| let filteri predicate (source: IObservable<'T>) = | |||
| Observable.Where( source, Func<_,_> predicate ) | |||
| Observable.Where( source, Func<_,_,_> (fun i x -> predicate x i) ) | |||
There was a problem hiding this comment.
I think we should have a test or something for this...
There was a problem hiding this comment.
Yes, and great catch!
|
Hmmm, so I was thinking of bringing in FsCheck to do the tests but to do so I need to upgrade the project to .NET 4.5. Any objections? |
|
by all means please do |
|
👍 |
Changed target framework to 4.5.2 Updated FSharp.Core to version 4.0.0.1 Updated various dependencies to their latest version
|
Anything else needed before this is ready to be merged in? Sorry: I don't have a lot of experience contributing to OSS and doing unit testing.... |
| @@ -9,7 +9,7 @@ | |||
| <OutputType>Library</OutputType> | |||
| <RootNamespace>FSharp.Control.Reactive</RootNamespace> | |||
| <AssemblyName>FSharp.Control.Reactive</AssemblyName> | |||
| <TargetFrameworkVersion>v4.0</TargetFrameworkVersion> | |||
| <TargetFrameworkVersion>v4.5.2</TargetFrameworkVersion> | |||
There was a problem hiding this comment.
@panesofglass I'm cool with upgrading the framework version to 4.5.2 and FSharp.Core to 4.0 if you are
|
FSharp Core 4.0 works with Framework 4.0. I'm not using this library at this moment, but I do maintain software that needs to ship for .NET 4.0. If you are changing the framework just because of most recent FSharp.Core that might not be the right decision. Maybe tests can be put in a separate project which targets 4.5.2? |
|
@smoothdeveloper valid point: there should be no need for the main project to change, only the test project. I think it was FsCheck that required 4.5. I will roll back the main project to 4.0 when I get a chance. |
|
@anton-pt once you fix the framework settings this is good to merge |
Reverted to .NET framework 4.0 for main project while keeping the test project on version 4.5.2 in order to use FsCheck. Updated dependencies.
Corrected the definition of Observable.filteri which previously only took a predicate of type 'T -> bool and simplified the definition of Observable.mapi, also improving performance.