-
Notifications
You must be signed in to change notification settings - Fork 359
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
Enhancement: marbles syntax + testing functions #299
Conversation
Really nice work! Questions:
|
Yes, this is the big drawback of one character = one timespan, you have to use the lookup dict if your value cannot be represented by only one character. In the contrary, it makes possible to emit consecutive values. Otherwise you would have to write "-4-2-", and divide by two your timespan. Do you think we lose too much usability with this change?
You're right, In a normal/test context, we could just alias Regarding
Yes, |
I've come to a solution for creating a hot observable with The basic idea is to provide a scheduler to I think we should also get rid of the subscription symbol '^'. It complicates things and I don't see what the purpose of it, even in tests. Don't know why they have introduced it in rxjs. What do you think ? |
Yes, if alpha numerical values are allowed as item values (i.e. [a-zA-Z0-9]), then we must use special characters for signaling.
We could separate groups by timespans (-) and still consider each digit as a timespan. For example "-42-3-" would mean that 42 is emitted at 1ts and 3 at 4ts. I even find this more readable because each emitted item is clearly separated from the previous/next ones. Also such a syntax could be reusable for ascii-to-marble docs because timespans would be consistent on multiple lines. For example something like this:
is quite readable on ascii, and timelines alignment is preserved on both observables even though they emit items of different "length". This is the current syntax that I consider for documentation now. Such a syntax has pros for documentation purpose, but may not be applicable for testing and serialization: On the previous example, an additional timespan is needed after the 9 value to keep text alignment with the second observable. So at the end I am not sure that we can use the very same syntax for doc and serialization/tests.
I agree. Also I do not see when cold vs hot observable difference is needed here (especially if we remove the subscription symbol). |
Agree to remove subscription symbol. It's important that we don't add features before we need them. I think the map times 2 example to @MainRo is very interresting since we easily get into the problem of how to represent multi-digit numbers, and I think it would be strange if
Q: Is the plan to fix |
Agree with @MainRo too, it should be a good compromise. There's still a case we need to clarify:
So 12, 4, 5 will be pushed at 2ts, and 6 at 12ts.
@dbrattli no, it was not planned. It should be better to make a later PR since the current PR may be too heavy. I was thinking about moving with context as (start, cold, hot, exp):
e0 = cold("--a--b--|")
ex = exp(" --a--b--|")
results = start(e0)
assert results == ex Of course, |
…rbles to rx.core.observable.marbles + update tests & examples
@dbrattli @MainRo What should we do with negative numbers ? Negative sign conflits with our time progression symbol. Maybe we can choose an other symbol for time progression e.g. underscore or tilde ?
Or just restricting to positive numbers and forcing the use of the lookup dict for negative numbers. |
Stick to using
|
This may be another reason why cold/testing syntax may not be exactly the same than for documentation: For documentation purpose there is no need to encode any possible values, this is why I proposed to restrict values to "[a-zA-Z0-9]". For cold/testing we probably want to represent more values but it will still be somehow limited (no objects, no bytes...). |
@@ -13,3 +15,5 @@ | |||
on_completed=lambda: print('good job!'), | |||
scheduler=ccy.timeout_scheduler, | |||
) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered using .run()
instead? It's made for exactly this scenario. It will then return the last value emitted, and you can pipe to ops.to_iterable()
to catch them all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right, good idea !
…observable/test_marbles.py
ops.flat_map(lambda value: observableLookup[value]), | ||
) | ||
|
||
source.subscribe_( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use subscribe()
from examples now that subscribe supports (again) both observers and callbacks. Subscribe with an _ will still be used by the operators, but it may be confusing for the users that we have two different functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted 👍
@dbrattli By the way, I've reworked all examples + some others things waiting in a big commit. I'm sorry this PR is a bit of a mess because I've pushed a lot of commits that changes everything. Do you prefer I close this one and open a new one when it's ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcafhe I think it's time to squash and merge. This feature is self contained and I'm not worried if it needs more work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool !
make marble context a true python context update tests update examples
Alright, sorry for the multiple commits, I've had to rearange a lot of things (and made some mistakes).
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR is for updating the marbles syntax, and providing some helper functions (
cold
&hot
) that mimic the rxjs functions. Some tests and examples have also been added.Sorry for the following wall of text 😕
Marbles syntax
In the following, "ts" stands for timespan. E.g. 3ts = 3 * timespan.
breaking changes
each character advance time by 1ts (except for space), e.g.
-ab-c--
will produceab
at 1ts,c
at 4ts instead ofab
at 1ts,c
at 3ts.change error symbol from
x
orX
to#
move
from_marbles
torx
(implemented in rx/core/observable/marbles.py). Aliascold
.move
to_marbles
torx.operators
(implemented in rx/core/operators/tomarbles.py)new features
add
rx.hot
(implemented in rx/core/observable/marbles.py)add support for grouping marbles e.g.
--(ab,c)-d-
. Every marble in a group share the same timestamp defined by the first(
. Each character (even parentheses) advance time by 1ts.ab
andc
will be emitted at 2ts,d
at 9ts.add optional lookup dictionnary to convert a marble to the specified value. E.g.
from_marbles("a-b--", lookup={'a':11, 'b':22}
will produce11
at 0 and22
at 2 * timespan.add optional error parameter to specify the exception raised by
#
marble.New functions
marbles_testing(timespan)
Python context that setup a TestScheduler and returns the functions below as a namedtuple. This ensures that everything will be called with the same values for the following parameters: subscribed, created, disposed, timespan. Parameters are set to:
Regarding
hot()
function, if a marble is specified as the first character, it will be skipped.cold(string, [lookup, error]) -> Observable
Parse a marbles string and return a cold observable by calling
rx.cold
.hot(string, [lookup, error]) -> Observable
Parse a marbles string and return a hot observable by calling
rx.hot
.start(create | observable) -> List[Recorded]
Wrapper around
TestScheduler.start()
. Returns a list of records instead of a MockObserver (i.e. mockobserver.messages).exp(string, [lookup, error]) -> List[Recorded]
Parse a marbles string and return a list of records.