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

refactor: Remove un-needed logic in EqualityComparable.__ne__ #2158

Merged
merged 1 commit into from Jul 1, 2023

Conversation

Sengolda
Copy link
Contributor

@Sengolda Sengolda commented Jul 1, 2023

Summary

Instead of __ne__ checking everything __eq__ already checks, just return the reverse boolean of __eq__ in __ne__

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

Signed-off-by: Sengolda <79252176+Sengolda@users.noreply.github.com>
@Lulalaby Lulalaby enabled auto-merge (squash) July 1, 2023 16:18
@Lulalaby Lulalaby merged commit 20598d4 into Pycord-Development:master Jul 1, 2023
25 checks passed
@Dorukyum
Copy link
Member

Dorukyum commented Jul 1, 2023

This is a de-optimization, isn't it? There's a reason that method was written explicitly after all.

Plus, where's the changelog entry?

@OmLanke
Copy link
Contributor

OmLanke commented Jul 1, 2023

This is a de-optimization, isn't it? There's a reason that method was written explicitly after all.

The logic is actually the same. __eq__ is just inline instead of separate lines and if blocks. So it should actually be more optimised. But there is no need to override __ne__. Python does it automatically. See https://stackoverflow.com/a/30676267. The method itself could be removed.

Plus, where's the changelog entry?

In my opinion, changelog should be only added for changes affecting library users. For library developers, the commit history itself is the changelog

@Dorukyum
Copy link
Member

Dorukyum commented Jul 1, 2023

The logic is actually the same. __eq__ is just inline instead of separate lines and if blocks. So it should actually be more optimised. But there is no need to override __ne__. Python does it automatically. See https://stackoverflow.com/a/30676267. The method itself could be removed.

I thought running a function and taking the inverse boolean value would be slower than just making the comparison with the code previously written there. Nevertheless, if it's not going to be written explicitly, we might as well remove the method like you said.

In my opinion, changelog should be only added for changes affecting library users. For library developers, the commit history itself is the changelog

Yeah you're right, my bad. My brain was hardcoded into thinking every change needed a changelog entry after we had so many pull requests without one.

@Lulalaby
Copy link
Member

Lulalaby commented Jul 1, 2023

Followup by #2159

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.

None yet

5 participants