Handle non-Job objects in Job equality check.

[bdice]

Description

I caught a couple minor errors in signac-flow tests (to be addressed separately) that were exposed by the changes in #442. This unrelated problem in signac-flow indicated an issue in the new __eq__ check, which doesn't correctly handle comparisons to non-Job objects. This PR fixes that bug so the Job class correctly adheres to the Python data model.

Specifically, the old __eq__ behavior did nothing to ensure that the objects being tested for equality were Job objects. If a job object and a tuple were compared, the previous Job equality test just checked if hash(job) == hash(tuple_of_data), which hid the test failures in signac-flow until #442 was merged and errors were raised by the comparison. However, the comparison should not raise errors or return False, and should instead return NotImplemented according to the Python data model and some StackOverflow posts I found.

Motivation and Context

Fixes minor bug in __eq__ implementation.

This issue from another library and this section of the Python documentation are helpful for explaining the motivation of the change.

I added tests that cover a range of cases.

Types of Changes

• Documentation update
• Bug fix
• New feature
• Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

• I am familiar with the Contributing Guidelines.
• I agree with the terms of the Contributor Agreement.
• My name is on the list of contributors.
• My code follows the code style guideline of this project.
• The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

• I have updated the API documentation as part of the package doc-strings.
• I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
• I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

[codecov]

Codecov Report

Merging #455 (6f72370) into master (8cfa82d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #455   +/-   ##
=======================================
  Coverage   77.13%   77.14%           
=======================================
  Files          42       42           
  Lines        5738     5740    +2     
  Branches     1119     1120    +1     
=======================================
+ Hits         4426     4428    +2     
  Misses       1027     1027           
  Partials      285      285           

Impacted Files	Coverage Δ
signac/contrib/job.py	91.20% <100.00%> (+0.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cfa82d...6f72370. Read the comment docs.

[atravitz]

Why does this ensure that we have NotImplemeted as opposed to False?

[bdice]

I'm testing both a != b and b != a because of the order in which Python resolves __eq__ operators. This section of the Python docs explains why both tests are relevant here, and why we return NotImplemented here: object.__eq__ docs.

Specifically, the order of resolution is controlled by this rule: "If the operands are of different types, and right operand’s type is a direct or indirect subclass of the left operand’s type, the reflected method of the right operand has priority, otherwise the left operand’s method has priority."

[bdice]

Also, I used this issue as a reference implementation because it appeared to correct the equality behavior that I was unsure about for subclasses (JobSubclass) and objects of different types with similar APIs (NonJob): googleapis/google-cloud-python#3455

[tcmoore3]

Good find, and nice fix!