Skip to content

Removed bogus .Filter() overload #1013

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JakenVeina
Copy link
Collaborator

@JakenVeina JakenVeina commented May 29, 2025

Removed a bogus .Filter() overload that did not allow the consumer to supply filtering logic, resulting in all items always being filtered out.

This appears to have been mistakenly introduced during a refactor, in commit 3657fee.

This is a breaking change that we can just include the next time we happen to have a major version release. I don't think it's worth doing a cycle of marking this deprecated, as the operator is just fully-defective.

…upply filtering logic, resulting in all items always being filtered out.
@JakenVeina JakenVeina self-assigned this May 29, 2025
@JakenVeina JakenVeina added Housekeeping Pull requests for minor code maintenance issues breaking change Items that contain a breaking change to the codebase labels May 29, 2025
@JakenVeina JakenVeina requested review from RolandPheasant and removed request for ChrisPulman May 31, 2025 01:51
@dwcullop
Copy link
Member

Is it a breaking change if no one is using it? If it does not work, how could anyone be using it?

Better safe than sorry, I guess.

@RolandPheasant
Copy link
Collaborator

Normally with a breaking change we would bump the major version. However in this case, the overload is meaningless so should we keep the major version or bump? Thoughts?

@JakenVeina
Copy link
Collaborator Author

I'm a "technically correct is the best kind of correct" kinda person, so I say keep it a major bump. Except, I don't think there's any need to bother bumpibg it ONLY for this. I'd just have us hold onto it until the next time we have something to trigger a major bump that's actually meaningful.

Copy link
Member

@dwcullop dwcullop left a comment

Choose a reason for hiding this comment

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

LGTM. Let's include it in the next major version bump release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Items that contain a breaking change to the codebase Housekeeping Pull requests for minor code maintenance issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants