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

Core: Added classification check to Item Equality #3032

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

agilbert1412
Copy link
Collaborator

@agilbert1412 agilbert1412 commented Mar 26, 2024

What is this fixing or adding?

Item equality comparison did not consider classifications. Since the case where the same item exists in many instances, with different classifications, is supported (and even often encouraged), this is a flaw in the system, where equality comparisons can get it wrong sometimes, for example removing or filtering the wrong items from collections

How was this tested?

Ran all the unit tests, plus my case that had a problem because of this

Discord Conversation: https://discord.com/channels/731205301247803413/731214280439103580/1222014497552597054

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Mar 26, 2024
@agilbert1412 agilbert1412 added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Mar 26, 2024
@Alchav
Copy link
Contributor

Alchav commented Mar 26, 2024

I wonder if a better solution might be checking that self is other

Seems like this would solve a lot of issues that have come up where .remove() removes the wrong instance of an item from a list, etc? Not sure if it might break anything anywhere though.

@agilbert1412
Copy link
Collaborator Author

It has been discovered, shortly after the creation of this PR, that plando and start inventory from pool rely on this "incomplete" equality to work properly.

These would need to get fixed as well, for this PR to be acceptable.

This is now out of my area of expertise, so I'm leaving it to anyone who wishes to bother. I'll be working around the bug from now on

@BadMagic100
Copy link
Collaborator

Incidentally I think self is other would cause issues here as well

@beauxq
Copy link
Collaborator

beauxq commented Mar 29, 2024

If this changes, the classification should also be added to __hash__

@Exempt-Medic Exempt-Medic added waiting-on: other Issue/PR is waiting for something else, like another PR. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Apr 27, 2024
@ThePhar ThePhar changed the title Core - Added classification check to Item Equality Core: Added classification check to Item Equality Jun 3, 2024
@ThePhar ThePhar added the meta: help wanted Additional review/assistance is requested for these issues or pull requests. label Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. meta: help wanted Additional review/assistance is requested for these issues or pull requests. waiting-on: other Issue/PR is waiting for something else, like another PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants