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

AggregateManager is not generic in error #192

Merged
merged 1 commit into from
May 8, 2024
Merged

AggregateManager is not generic in error #192

merged 1 commit into from
May 8, 2024

Conversation

angelo-rendina-prima
Copy link
Contributor

@angelo-rendina-prima angelo-rendina-prima commented Apr 25, 2024 •
edited
Loading

Addresses #188

@angelo-rendina-prima angelo-rendina-prima requested a review from a team as a code owner April 25, 2024 14:59
MaeIsBad
MaeIsBad reviewed Apr 26, 2024
View reviewed changes
Copy link
Member

@MaeIsBad MaeIsBad left a comment

Choose a reason for hiding this comment

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

This seems like a way cleaner/idiomatic way to do this, but I'm not sure if this is worth breaking existing codebases over.
Maybe we could make a new handle_command_v2 function(name TBD) and deprecate the old one? WDYT?

@angelo-rendina-prima angelo-rendina-prima marked this pull request as draft April 28, 2024 17:21
@angelo-rendina-prima angelo-rendina-prima force-pushed the handle-command-is-not-generic-in-error branch from 02dfd7b to bf4d405 Compare May 7, 2024 13:40
@angelo-rendina-prima angelo-rendina-prima force-pushed the handle-command-is-not-generic-in-error branch from bf4d405 to f4d43de Compare May 7, 2024 15:57
@angelo-rendina-prima angelo-rendina-prima marked this pull request as ready for review May 8, 2024 07:47
@angelo-rendina-prima
Copy link
Contributor Author

This seems like a way cleaner/idiomatic way to do this, but I'm not sure if this is worth breaking existing codebases over. Maybe we could make a new handle_command_v2 function(name TBD) and deprecate the old one? WDYT?

We are still in 0.x so any change can be breaking. It is unfortunate but the old API was broken anyway (not allowing for generics in the target Into). Also we are including #193 in the 0.16 release, which is breaking too 🤷

@cottinisimone cottinisimone merged commit 627585f into master May 8, 2024
7 checks passed
@cottinisimone cottinisimone deleted the handle-command-is-not-generic-in-error branch May 8, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaeIsBad MaeIsBad MaeIsBad left review comments

@cottinisimone cottinisimone cottinisimone approved these changes

@Johnabell Johnabell Awaiting requested review from Johnabell

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

Successfully merging this pull request may close these issues.
3 participants
@angelo-rendina-prima @cottinisimone @MaeIsBad