-
Notifications
You must be signed in to change notification settings - Fork 819
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
Signal #4012
Signal #4012
Conversation
@@ -248,11 +248,14 @@ def get_token(self, expect: Expect) -> Token: | |||
line = self.lines[line_no] | |||
match = expect.match(line, col_no) | |||
if match is None: | |||
error_line = line[col_no:].rstrip() |
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.
Slight tweak to formatting. Checks if there is a semi-colon at the end of the line, before suggesting it.
@davep You may be able to use the Signal mechanism herein for the suspend resume notification. |
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.
Looks good to me. Not 100% on the name - mostly "publish" sounds a little strange to me (publish a signal?)
Any suggestions for an alernative? |
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.
One or two suggested tweaks to the ChangeLog, a question about the interface, but on the whole I subscribe to this PR.
@@ -77,7 +77,7 @@ def __init_subclass__( | |||
cls.handler_name = f"on_{namespace}_{name}" if namespace else f"on_{name}" | |||
|
|||
@property | |||
def control(self) -> Widget | None: | |||
def control(self) -> DOMNode | None: |
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.
Possibly want to tweak the docstring to say node rather than widget now; or from the "end user" point of view does the intent of the property remain the same? (we want them to think about widgets)
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.
Yeah, might leave it as widget in the docs. It's more meaningful for the average user.
if callback not in callbacks: | ||
callbacks.append(callback) | ||
|
||
def unsubscribe(self, node: DOMNode) -> None: |
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.
As I read the interface, it seems you can subscribe multiple callbacks from a single node, but on the other hand you can only unsubscribe a node (presumably unsubscribing every callback). Is that a correct reading and is that intentional?
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.
That's intentional. It's more to clean up all subscriptions on a node. If its a requirement, we can do something more finely grained later.
Co-authored-by: Dave Pearson <[email protected]>
This is mainly for a new Signal class, and also some fixes / new method