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

Create 1-many and many-many versions of DerivedSignal #661

Merged
merged 17 commits into from
Mar 24, 2025
Merged

Create 1-many and many-many versions of DerivedSignal #661

merged 17 commits into from
Mar 24, 2025
+1,214 −27

Conversation

coretl
Copy link
Collaborator

@coretl coretl commented Nov 21, 2024 •
edited
Loading

Fixes #525

@coretl coretl requested a review from evvaaaa November 21, 2024 14:22
@coretl coretl mentioned this pull request Feb 4, 2025
Open
2 tasks
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 5, 2025
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 12, 2025
View reviewed changes
@coretl coretl marked this pull request as ready for review March 19, 2025 11:54
@coretl
Copy link
Collaborator Author

coretl commented Mar 19, 2025

Missing a few "unhappy path" tests, but otherwise ready to review

@coretl coretl requested review from evvaaaa and DominicOram March 19, 2025 11:55
DominicOram
DominicOram requested changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Thanks! I have just reviewed the tests as an example of how it would be used downstream and don't have time to review the implementation, sorry. Some of my comments may be a bit harsh/nitpicky but I was trying to review it from the perspective of "these are documented example cases that are the gold standard for how to use it".

Most of the comments are optional but I would like docs on this somewhere before I would approve

This was referenced Mar 21, 2025
Open
Open
@coretl coretl mentioned this pull request Mar 21, 2025
Open
@coretl coretl changed the title Initial stab at DerivedSignal #525 Create 1-many and many-many versions of DerivedSignal Mar 21, 2025
@coretl coretl requested a review from DominicOram March 21, 2025 16:55
DominicOram
DominicOram approved these changes Mar 24, 2025
View reviewed changes
Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Great, thanks! I still haven't looked at the implementation in detail but the interface now looks good, just some minor suggestions, take them or leave them

This was referenced Mar 24, 2025
Open
Merged
@coretl coretl requested a review from DominicOram March 24, 2025 16:50
DominicOram
DominicOram approved these changes Mar 24, 2025
View reviewed changes
Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

One nit, otherwise looks good. Thanks! If you get this into a release I will try and get and convert some of our usecases in dodal

@coretl coretl merged commit befee42 into main Mar 24, 2025
2 checks passed
@coretl coretl deleted the derived-signal branch March 24, 2025 17:10
@DominicOram DominicOram mentioned this pull request Mar 25, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DominicOram DominicOram DominicOram approved these changes

@evvaaaa evvaaaa Awaiting requested review from evvaaaa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Uses cases for signals that derive from other signals
3 participants
@coretl @DominicOram @evvaaaa