Skip to content

Revamp modules structure #235

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.

Sign up for GitHub

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

Jump to bottom
Merged
llucax merged 13 commits into from
Nov 9, 2023
Merged

Revamp modules structure #235

llucax merged 13 commits into from
Nov 9, 2023

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Nov 7, 2023 •
edited
Loading

  • Remove Bidirectional channel
  • Remove Peekable and associated methods/classes
  • Don't import TypeVars from other modules
  • Make all TypeVars private
  • Split _base_classes into _receiver and _sender
  • Make channels return the base sender and receiver types
  • Make channel's sender and receiver implementation private
  • Don't use aliases for the base sender and receiver classes
  • Move merge and select symbols to the top level
  • Move utility receivers to their own public modules
  • Move nested class out of the parent class

Fixes #233, fixes #234.

@llucax
Remove Bidirectional channel
db1a80d 
Having bidirectional channels is not a recommended practice, as it could
introduce deadlocks.  It is a very niche use case, so it is better to
remove it.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax self-assigned this Nov 7, 2023
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:channels Affects channels implementation part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) labels Nov 7, 2023
@llucax llucax added type:enhancement New feature or enhancement visitble to users scope:breaking-change Breaking change, users will need to update their code part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:channels Affects channels implementation part:synchronization Affects the synchronization of multiple sources (`select`, `merge`) part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) and removed part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:channels Affects channels implementation part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) labels Nov 7, 2023
llucax added 10 commits November 7, 2023 16:11
@llucax
Remove Peekable and associated methods/classes
6f73235 
`Peekable` was a class that allowed users to peek at the latest value in
a channel, without consuming anything. Even when useful in principle, it
is a special case that breaks the whole channel abstraction, and can
easily be emulated by just using a normal receiver and caching the last
value.

Also the following symbols are removed as they are not longer used:

* 'Broadcast.new_peekable`
* `Receiver.into_peekable`
* `ReceiverInvalidatedError`

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Don't import TypeVars from other modules
e1358ec 
There is little gain in importing type variables from other modules, as
it adds unncecessary dependencies between modules. Type variables are
just an artifact to define stuff locally.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Make all TypeVars private
cb68d99 
Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Split _base_classes into _receiver and _sender
f7c86cc 
Also move the receiver exceptions from `_exceptions` to `_receiver` and
the sender exceptions from `_exceptions` to `_sender`.

This avoids circular imports.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Make channels return the base sender and receiver types
805b70f 
We don't really want end users to have access to the concrete types, and
their custom methods. We want them to use the base types, so we can
change the implementation without breaking their code and ensuring
a common interface for all senders and receivers.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Make channel's sender and receiver implementation private
1708cb2 
We don't want to expose the implementation of the sender and receiver at
all, so we make them private to the module.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Don't use aliases for the base sender and receiver classes
b17803d 
Now that the sender and receiver implementations are private, there is
no need to use aliases for the base classes.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Move merge and select symbols to the top level
c3951f7 
These are core component to work with channels, so they are moved to the
`frequenz.channels` top level module.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Move utility receivers to their own public modules
25b38ba 
We move the `Event`, `FileWatcher` and `Timer` receivers to their own
separate public modules. Since they are not core components and users
might only need to use some of them, it is more clear to have them
separated.

Also in the case of the `FileWatcher`, it requires an extra dependency,
which we could potentially have as an optional dependency, so users that
don't need it don't need to install it.

In the future we might distribute these also as separate python packages
(wheels).

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Move nested class out of the parent class
dd756d7 
Removing nested classes avoids having to use hacky constructions, like
requiring the use of `from __future__ import annotations`, types as
strings and confusing the `mkdocstrings` tools when extracting and
cross-linking docs.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax mentioned this pull request Nov 8, 2023
Closed
llucax added 2 commits November 8, 2023 14:12
@llucax
Remove release notes Summary
8e70c41 
This is from the beta-2 release, so we clear it out until we are ready
for the new release.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Update release notes
cc17298 
Signed-off-by: Leandro Lucarella <[email protected]>
@github-actions github-actions bot added part:docs Affects the documentation and removed part:synchronization Affects the synchronization of multiple sources (`select`, `merge`) labels Nov 8, 2023
@llucax
Copy link
Contributor Author

llucax commented Nov 8, 2023

Added release notes, this is ready for the final review.

@llucax llucax marked this pull request as ready for review November 8, 2023 14:04
@llucax llucax requested a review from a team as a code owner November 8, 2023 14:04
@llucax llucax requested a review from Marenz November 8, 2023 14:04
shsms
shsms reviewed Nov 9, 2023
View reviewed changes
@shsms
Copy link
Contributor

shsms commented Nov 9, 2023

Just one comment, looks good otherwise.

@llucax llucax added this pull request to the merge queue Nov 9, 2023
Merged via the queue into frequenz-floss:v1.x.x with commit 39ade4e Nov 9, 2023
@llucax llucax deleted the restructure branch November 9, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shsms shsms shsms approved these changes

@Marenz Marenz Awaiting requested review from Marenz Marenz is a code owner automatically assigned from frequenz-floss/python-sdk-team

Assignees

@llucax llucax

Labels

part:channels Affects channels implementation part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) scope:breaking-change Breaking change, users will need to update their code type:enhancement New feature or enhancement visitble to users

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove Bidirectional Remove Peekable

2 participants

@llucax @shsms