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

fix(CVSS2, CVSS3, CVSS4): implement and correct __eq__ methods #62

Conversation

fqlenos
Copy link
Contributor

@fqlenos fqlenos commented Sep 12, 2024

This PR addresses the following:

  • Implement the missing __eq__ method in the CVSS4 class, which was necessary to enable proper object comparison. Without this, comparing two CVSS4 objects would not work correctly.
  • Correct and refactor the __eq__ methods in CVSS2 and CVSS3 to ensure consistency across all CVSS classes.

These changes resolve issues with object comparisons and ensure that all CVSS classes now follow consistent behavior when comparing instances.

@skontar skontar requested a review from jsvob September 12, 2024 20:25
Removed type hints from the __eq__ and __hash__ magic methods in CVSS2, CVSS3 and CVSS4 classes for better compatibility with < Python3.5 versions
Copy link
Collaborator

@jsvob jsvob left a comment

Choose a reason for hiding this comment

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

Hi @fqlenos ! Thank you for your contribution.

Why the changes from return NotImplemented to return False, please? I admit I've never implemented comparison, but don't have the spare cycles now to explore it fully.

@fqlenos
Copy link
Contributor Author

fqlenos commented Sep 16, 2024

Hello @jsvob!

The change was made for the following reasons:

  1. Behavioral consistency: return False directly communicates that the comparison failed, which I think aligns with the expected behavior when comparing two instances.
  2. Reverse comparison: return NotImplemented signals Python to try other comparison method, such as the reverse comparison (o.clean_vector() == self.clean_vector()) if available.
  3. Practical impact: return False is clearer in contexts where comparisons are used directly, such as if CVSS3(...) == CVSS3(...): .... Using return NotImplemented would delegate comparison to other __eq__ method, which could lead to unexpected behavior if not handled correctly.

In short, and in my opinion, returning False here simplifies and ensures a clear outcome when two objects are not equal or are of different types, without relying on additional fallback logic.

@jsvob
Copy link
Collaborator

jsvob commented Sep 16, 2024

Thank you for the detailed explanation and fix, @fqlenos !

@fqlenos fqlenos closed this Sep 16, 2024
@fqlenos fqlenos reopened this Sep 16, 2024
@skontar skontar merged commit 5e83877 into RedHatProductSecurity:master Sep 16, 2024
3 of 10 checks passed
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.

3 participants