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

Drop explicit sender attribute from messages #1940

Merged
merged 18 commits into from
Mar 6, 2023
Merged

Drop explicit sender attribute from messages #1940

merged 18 commits into from
Mar 6, 2023

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Mar 5, 2023

Rectifies two API issues.

The sender attribute create boiler plate when sending messages. It was also a source of confusion as to its purpose.

Having two messages post_message and post_message_no_wait is rather ugly, and difficult to explain. Awaiting posting something on a message queue is only necessary if the queue is bounded, but they aren't any more.

  • Dropped sender from the Message base class.
  • Dropped the async post_message. There is now just post_message which is syncronous.
  • Added a control alias to messages from controls.

See CHANGELOG for details

@willmcgugan willmcgugan marked this pull request as draft March 5, 2023 09:36
@willmcgugan willmcgugan marked this pull request as ready for review March 5, 2023 13:07
@davep
Copy link
Contributor

davep commented Mar 5, 2023

I'll review properly tomorrow morning, if this doesn't go in beforehand, but much like with the alternate compose method I'm throwing a very hearty OH HELL YES at this.

CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@davep davep left a comment

Choose a reason for hiding this comment

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

Couple of tweaks needed around ToggleButton and friends, but otherwise looks good. This is a really nice spring-clean!

self.post_message_no_wait(
self.Changed(self, cast(RadioButton, event.input))
)
self.post_message(self.Changed(self, cast(RadioButton, event.radio_button)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect we don't need this cast now.

super().__init__(sender)
self.input = sender
super().__init__()
self._toggle_button = toggle_button
"""A reference to the toggle button that was changed."""
Copy link
Contributor

Choose a reason for hiding this comment

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

This docstring can go now.

"""A reference to the toggle button that was changed."""
self.value = value
"""The value of the toggle button after the change."""

@property
def control(self) -> ToggleButton:
"""Alias for self.toggle_button."""
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need a reword given that toggle_button was moved to being "private". Actually, I'm wondering if control should move to the two individual child message classes.

@willmcgugan willmcgugan merged commit 373fc95 into main Mar 6, 2023
@willmcgugan willmcgugan deleted the drop-sender branch March 6, 2023 10:52
@rodrigogiraoserrao rodrigogiraoserrao linked an issue Mar 6, 2023 that may be closed by this pull request
rodrigogiraoserrao added a commit that referenced this pull request Mar 6, 2023
In the aftermath of #1935 and #1940 this fell through the cracks.
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.

RadioButton.Change event should have a button attribute
2 participants