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

fix: add missing dependencies params for liveness scope and table listener hooks #291

Merged
merged 6 commits into from
Mar 1, 2024

Conversation

niloc132
Copy link
Member

Fixes #289

@niloc132 niloc132 force-pushed the 289-hook-dependencies branch from be2ffa1 to 41f4e66 Compare February 16, 2024 20:08
@mofojed mofojed marked this pull request as ready for review February 22, 2024 21:31
@mofojed
Copy link
Member

mofojed commented Feb 22, 2024

My only real comments would be that it might be nice to accept a list or a set for deps. I don't feel too strongly about it, but would be more familiar to do [] as opposed to set() in scenarios with empty deps, e.g. use_memo(lambda: 'foo', []) than use_memo(lambda: 'foo', set()).

@jnumainville this looks good to me, what do you think?

@jnumainville
Copy link
Collaborator

I agree, I think we can go so far as to allow any Sequence. I made this edit on my type checking review

@mofojed
Copy link
Member

mofojed commented Feb 22, 2024

Okay, lets get your PR merged first, then I'll rebase this one on top.

mofojed
mofojed previously approved these changes Feb 29, 2024
jnumainville
jnumainville previously approved these changes Feb 29, 2024
- Also added crash logs to gitignore
@mofojed mofojed dismissed stale reviews from jnumainville and themself via 4c0bc7b February 29, 2024 20:30
@mofojed mofojed merged commit 9d6b7de into deephaven:main Mar 1, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use_liveness_scope and use_table_listener hooks should require dependencies
3 participants