minamorl / rex

Observable pattern in nim
22 stars 1 forks source link

Reimplement #9

Closed PhilippMDoerner closed 4 months ago

PhilippMDoerner commented 4 months ago

Second attempt at a merge. This time also implementing the take operator and async! I'll copy the description of the last PR since that still applies:

A reimplementation of the project, this fixes https://github.com/minamorl/rex/issues/7 and a bunch of other things.

It changes the entire approach fundamentally. You no longer have procs that work on observables. You have observables that store closure functions which typically bind the observable itself and whatever observer is supposed to be used on it.

This approach creates a lot of circular datastructures (and thus requires folks to use ORC to deal with those cycles).

In exchange, it made the following possible to implement:

You can now "unsubscribe" from arbitrarily complex observables
Subjects and Observables are separated
You now have error handling
Observables can be defined using procs as in the initial implementation
Likely way more efficient because observables etc. actually act lazily
You now can register "complete" procs when subscribing
You can complete subjects
Moves subjects into their own module. Adding new subjects will be rather easy this way because all it requires is defining inheriting from Observable and defining similar procs that Subject also does, likely with adjustments to the closures that it stores
Moves operators into their own module. Adding new operators will be rather easy this way because you can just use existing operators as a blueprint for adding more most of the time

I think this provides the fundamental baseline for being able to implement the entire spec. It seems pretty doable from here on out.

On top of that with the newest version:

PhilippMDoerner commented 4 months ago

What I'd need now is the following: 1) An explainer how to get the github workflow to work as intended (it should just run nimble test and that should do the trick) 2) A quick yes/no if that approach seems acceptable for the given project (You mentioned you wanted to be included when it comes to project direction. Including async is pretty significant in that part) 3) ... maybe some help with rewriting examples in the README.md and listing the capabilities that we have so far :smile: 4) Maybe some additional tests, I'm pretty sure the test-suite is not fully exhaustive, though it has been a really nice guide and helping hand so far

minamorl commented 4 months ago

For the test you can leave it now if it's hard for you to fix them. I think it can be fixed as other issue after this merged (You can remove current github acvtions for now)

minamorl commented 4 months ago

As far as I can see lgtm. README should be rewritten tho. I think we can do them as separated another issue. I am really appreciate your hard job! well done!

minamorl commented 4 months ago

hmm it seems installation issue more than test itself? https://github.com/minamorl/rex/actions/runs/8959850118/job/24605725104 (Edit: macos binary 2.0.4 seem missing for this action. I think we can ignore them)

"body": "Commit: [https://github.com/nim-lang/Nim/commit/b47747d31844c6bd9af4322efe55e24fefea544c\n\nGenerated](https://github.com/nim-lang/Nim/commit/b47747d31844c6bd9af4322efe55e24fefea544c/n/nGenerated) release binaries will be uploaded as they're made available."
PhilippMDoerner commented 4 months ago

Could you check if the windows implementation runs? It was previously only interrupted due to the macos one crashing

PhilippMDoerner commented 4 months ago

Okay, with the test-suite running through I'm comfortable merging this.