Skip to content

Conversation

MarvinKlein1508
Copy link
Collaborator

Pull Request

πŸ“– Description

This PR removes the ToArray call for the OnCompleted event callback which is not necessary. This saves a few allocations for the copied array.

🎫 Issues

πŸ‘©β€πŸ’» Reviewer Notes

πŸ“‘ Test Plan

βœ… Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new component
  • I have modified an existing component
  • I have validated the Unit Tests for an existing component

⏭ Next Steps

Copy link
Collaborator

@Tyme-Bleyaert Tyme-Bleyaert Sep 12, 2025

Choose a reason for hiding this comment

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

Is it not because we want to send a decoupled snapshot of the list?

Copy link
Collaborator Author

@MarvinKlein1508 MarvinKlein1508 Sep 12, 2025

Choose a reason for hiding this comment

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

This could be a reason. But I don't believe that nobody is going to manually cast the IEnumerable<T> into a List<T> and appends or remove items to it. I cannot think of any good reason why someone even should try to do this.

But this is up for the maintainers to decide. I just noticed it while debugging on another issue! :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I think it's better to keep the ToArray to ensure this decoupling.
It has small impact on performance and avoids potential problems that a user might encounter when manipulating this list in OnCompleted`` . Although I can't immediately think of a practical example of a problem :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change something that works? πŸ€ͺ

Copy link
Collaborator

@vnbaaij vnbaaij Sep 12, 2025

Choose a reason for hiding this comment

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

Closing this one on account of "if it ain't broke, dont fix it'...

@vnbaaij vnbaaij closed this Sep 12, 2025
@MarvinKlein1508 MarvinKlein1508 deleted the patch-inputfile branch September 12, 2025 11:57
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.

4 participants