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

Data binding and more #4075

Merged
merged 38 commits into from
Feb 6, 2024
Merged

Data binding and more #4075

merged 38 commits into from
Feb 6, 2024

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Jan 29, 2024

Bit of a mixed bag. Added a number of things while working on Tailless. The most notable is probably the data binding.

See changelog for details.

@willmcgugan willmcgugan marked this pull request as draft January 29, 2024 17:06
@willmcgugan willmcgugan changed the title added cancelled event to worker Data binding and more Feb 6, 2024
@willmcgugan willmcgugan marked this pull request as ready for review February 6, 2024 15:06
Copy link
Member

@darrenburns darrenburns left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Darren Burns <darrenburns@users.noreply.github.com>
@willmcgugan willmcgugan merged commit 5d6c61a into main Feb 6, 2024
20 checks passed
@willmcgugan willmcgugan deleted the cancelled-event branch February 6, 2024 16:41
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

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

I left a couple of questions for possible improvements.

Comment on lines +452 to +458
def set(
self,
display: bool | None = None,
visible: bool | None = None,
disabled: bool | None = None,
loading: bool | None = None,
) -> DOMQuery[QueryType]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this would take **kwargs and just set whatever keyword argument(s) is/are provided on all of the DOMNodes. Wouldn't that be more helpful than being prescriptive about the specific attributes we can set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's for the benefit of typechecking mainly.

Comment on lines +456 to +457
disabled: bool | None = None,
loading: bool | None = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

disabled and loading are attributes on widgets, not on DOM nodes. Won't that be a problem? Maybe due to typing issues or inconsistencies?

yield WorldClock("Europe/Paris").data_bind(WorldClockApp.time)
yield WorldClock("Asia/Tokyo").data_bind(WorldClockApp.time)
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +282 to +284

Args:
compose_parent: The node doing the binding.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Args:
compose_parent: The node doing the binding.

Comment on lines +290 to +306
def make_setter(variable_name: str) -> Callable[[object], None]:
"""Make a setter for the given variable name.

Args:
variable_name: Name of variable being set.

Returns:
A callable which takes the value to set.
"""

def setter(value: object) -> None:
"""Set bound data."""
_rich_traceback_omit = True
Reactive._initialize_object(self)
setattr(self, variable_name, value)

return setter
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this be defined outside of the for loop?

self.watch(
compose_parent,
reactive.name,
setter,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not partial(setattr, self, variable_name)?

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.

None yet

3 participants