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

Add switch_map operator and equivalent starred and indexed #634

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

giff-h
Copy link
Contributor

@giff-h giff-h commented Mar 13, 2022

This draws from the definition in rxjs and maintains parity with the
map operator and its variants

@coveralls
Copy link

coveralls commented Mar 13, 2022

Coverage Status

Coverage increased (+0.09%) to 93.449% when pulling 7fbbd9d on hamstap85:feat/switch_map_operator into a227802 on ReactiveX:master.

@dbrattli
Copy link
Collaborator

dbrattli commented Mar 14, 2022

Great work! The main building blocks of Rx is the merge functions (merge, switch_latest, concat) combined with the mapper forms the functions we know as flat_map (i.e map_merge), switch_map and concat_map. So we want to have switch_map as much as we want to have flat_map. We should also add concat_map.

This draws from the definition in rxjs and maintains parity with the
map operator and its variants
"""
The switch_map operator.

Project each element of an observable sequence into a new observable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Project each element of an observable sequence into a new observable.
Project each element of an observable sequence into a new observable. producing values only from the most
recent observable sequence.

The switch_map_indexed operator.

Project each element of an observable sequence into a new observable
by incorporating the element's index.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
by incorporating the element's index.
by incorporating the element's index and producing values only from the most recent
observable sequence.

Unpack arguments grouped as tuple elements of an observable sequence
and return an observable sequence whose values are each element of
the observable returned by invoking the mapper function with star
applied on unpacked elements as positional arguments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
applied on unpacked elements as positional arguments.
applied on unpacked elements as positional arguments, and only producing values only from the most recent
observable sequence.

An operator function that takes an observable source and returns
an observable sequence whose values are each element of the
observable returned by invoking the mapper function with the
unpacked elements of the source.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here: " ... producing values only from the most recent observable sequence.""" or else it looks like a flat_map.

@dbrattli
Copy link
Collaborator

@hamstap85 One thing that worries me about this PR is that the tests are derived from test_map and not e.g test_switchlatest. Thus the tests do not have the notion of time and behave more like a regular flat_map / concat_map in the sense that the switching property of switch_map is not tested. I.e the automatic disposal of the previous observable being subscribed. This is also reflected in the docstrings of the operators where the " producing values only from the most recent observable sequence" is missing. The current docstrings sounds more like a flat_map so I'm worried that your original impl also behaved more like a flat_map / concat_map. Sorry for being difficult, but I just need to be sure that you are implementing what you think you are implementing.

@giff-h
Copy link
Contributor Author

giff-h commented Mar 15, 2022

@dbrattli No worries at all, thanks for the feedback.

What's the desired max line length in docstrings? Equal to python code max length enforced by black?

@dbrattli
Copy link
Collaborator

dbrattli commented Mar 15, 2022

Docstrings should follow PEP-8, i.e 72 chars per line. PS: all the current tests for switch_map and switch_map_index pass if you replace switch_map with flat_map etc. The only difference is that switch_map uses of instead of identity as the default mapper function. But in my view they should either both use of or both use identity. Do you have a reference for another library using of for the default mapper? I'm not opposed for changing the default for flat_map but I cannot see how this is currently handled in e.g RxJS. FYI: https://www.baeldung.com/rxjava-flatmap-switchmap

@matiboy matiboy mentioned this pull request Jan 7, 2023
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.

3 participants