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.

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

Revamp modules structure #235

Merged
merged 13 commits into from
Nov 9, 2023
Merged

Revamp modules structure #235

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 <luca-frequenz@llucax.com>
@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
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 <luca-frequenz@llucax.com>
@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 <luca-frequenz@llucax.com>
@llucax
Make all TypeVars private
cb68d99 
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@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 <luca-frequenz@llucax.com>
@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 <luca-frequenz@llucax.com>
@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 <luca-frequenz@llucax.com>
@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 <luca-frequenz@llucax.com>
@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 <luca-frequenz@llucax.com>
@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 <luca-frequenz@llucax.com>
@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 <luca-frequenz@llucax.com>
@llucax llucax mentioned this pull request Nov 8, 2023
Closed
@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 <luca-frequenz@llucax.com>
@llucax
Update release notes
cc17298 
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@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
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
14 checks passed
@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