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

of and to functions, additional operators #6

Merged
merged 7 commits into from
Dec 23, 2024

Conversation

xperiandri
Copy link
Collaborator

@xperiandri xperiandri commented Dec 1, 2024

Proposed Changes

Add

  • empty
  • singleton
  • ofSeq
  • ofAsync
  • ofTask
  • toArray
  • toList
  • toLookup
  • catch
  • chunkBy
  • chunkBySize
  • merge
  • choose
  • ofType

Types of changes

What types of changes does your code introduce to FSharp.Control.R3?

New feature (non-breaking change which adds functionality)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • Build and tests pass locally
  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added the necessary documentation (if appropriate)

* `empty`
* `singleton`
* `ofSeq`
* `ofAsync`
* `ofTask`
* `toArray`
* `toList`
* `toLookup`
* `catch`
* `chunkBy`
* `chunkBySize`
* `merge`
* `choose`
@xperiandri xperiandri requested a review from Thorium December 1, 2024 21:39
@Thorium
Copy link
Member

Thorium commented Dec 1, 2024

How does the user know which of these are blocking and which not? (So a hot observable may block the thread and never return.)
For example:

  • ofSeq is not, it just publish existing sequence as observable.
  • toList is because observable has to be completed before it can be transformed as list.
  • empty... Is it running onComplete or not?
  • chinkBySize: I would expect it is not. But this is taking the "window" (size) from beginning, not from the end. So not useful for a "sliding window cache" (like "undo buffer of last 5 actions").

@xperiandri
Copy link
Collaborator Author

Maybe we should introduce Blocking’ submodule in the Observable’ module to make that explicit?

@Thorium
Copy link
Member

Thorium commented Dec 3, 2024

or maybe just be clear in xml comments?

src/FSharp.Control.R3/AsyncObservable.fs Outdated Show resolved Hide resolved
let inline merge (source1, source2) = ObservableExtensions.Merge (source1, source2)

/// Returns an observable sequence that contains only a single element
let inline singleton item = Observable.Return<'T> item
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate this "sigleton" because it reminds people of OO GoF desgin-pattern of singleton which this is not, this is not a static instance and there can be more by just merging this. This is rather "lift my value to this computation expression", I don't know if there are better more F#pish names like Option.ofObj or Task.FromResult.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/FSharp.Control.R3/Observable.fs Outdated Show resolved Hide resolved
@xperiandri
Copy link
Collaborator Author

What do you think about the remarks on modules?

@Thorium
Copy link
Member

Thorium commented Dec 8, 2024

Looks ok to me.

@@ -11,13 +12,38 @@ let inline bind ([<InlineIfLambda>] f : 'T -> Observable<'TNext>) source = Obser
/// Converts the elements of the sequence to the specified type
let inline cast<'T, 'CastType> (source) = ObservableExtensions.Cast<'T, 'CastType> (source)

let inline catch ([<InlineIfLambda>] f : 'Exn -> Observable<'TNext>) o = ObservableExtensions.Catch (o, f)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the R3 exception handling significantly differs from Rx, this could need some xml comments as well. (R3 docs: "Stopping the pipeline at OnError is a mistake.")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added XML comment with remarks

@xperiandri
Copy link
Collaborator Author

Added ofType, aligned type parameters naming

@xperiandri xperiandri merged commit a8b67eb into main Dec 23, 2024
7 checks passed
@xperiandri xperiandri deleted the of-and-to-functions-additional-operators branch December 23, 2024 21:50
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