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

Return none when draining new orders and returning an empty vec #180

Closed
bertmiller opened this issue Sep 15, 2024 · 1 comment
Closed

Return none when draining new orders and returning an empty vec #180

bertmiller opened this issue Sep 15, 2024 · 1 comment
Labels
C-debt Technical debt items that need to be addressed to improve code quality or maintainability good first issue Good for newcomers invalid This doesn't seem right

Comments

@bertmiller
Copy link
Member

IMO drain_new_orders() should return None if it has an empty vector. It's not very intuitive to me that it can return Some with an empty vector, which lead to a bug when integrating with a new algorithm.

Here's the relevant code.

@bertmiller bertmiller added good first issue Good for newcomers invalid This doesn't seem right C-debt Technical debt items that need to be addressed to improve code quality or maintainability labels Sep 15, 2024
@ZanCorDX
Copy link
Contributor

Disagree.

  • I don't like using None for empty vectors since it generates non uniform code and the empty vector is as valid as any other vector.
  • The semantic of the None here is very specific. It means that you received a remove so your groups are no longer valid (at the moment of the creation of this code we had no way to unmerge groups) and instead of trying to upgrade your groupings you should restart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-debt Technical debt items that need to be addressed to improve code quality or maintainability good first issue Good for newcomers invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants