Repository: glotzerlab/signac
Pull request #455 (2020-12-30 05:45:39)

Handle non-Job objects in Job equality check.
## 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](https://docs.python.org/3/reference/datamodel.html#object.__eq__) and some StackOverflow posts I found.

## Motivation and Context
Fixes minor bug in `__eq__` implementation.

[This issue from another library](https://github.com/googleapis/google-cloud-python/issues/3455) and [this section of the Python documentation](https://docs.python.org/3/reference/datamodel.html#object.__eq__) are helpful for explaining the motivation of the change.

I added tests that cover a range of cases.

## Types of Changes
<!-- Please select all items that apply either now or after creating the pull request: -->
- [ ] Documentation update
- [x] Bug fix
- [ ] New feature
- [x] Breaking change<sup>1</sup>

<sup>1</sup>The change breaks (or has the potential to break) existing functionality.

## Checklist:
<!-- Please select all items that apply either now or after creating the pull request. -->
<!-- If you are unsure about any of these items, do not hesitate to ask! -->
- [x] I am familiar with the [**Contributing Guidelines**](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [x] I agree with the terms of the [**Contributor Agreement**](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [x] My code follows the [code style guideline](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md#code-style) of this project.
- [x] 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](https://docs.signac.io/) on [signac-docs](https://github.com/glotzerlab/signac-docs) and linked it here.
- [x] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt) and added all related issue and pull request numbers for future reference (if applicable). See example below.

codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/455?src=pr&el=h1) Report
> Merging [#455](https://codecov.io/gh/glotzerlab/signac/pull/455?src=pr&el=desc) (f0404ba) into [master](https://codecov.io/gh/glotzerlab/signac/commit/c1cf74e4dbc792056a8b957689ba5c0b56c5ec7b?el=desc) (c1cf74e) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/glotzerlab/signac/pull/455/graphs/tree.svg?width=650&height=150&src=pr&token=FghbjTyzgM)](https://codecov.io/gh/glotzerlab/signac/pull/455?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #455   +/-   ##
=======================================
  Coverage   76.98%   76.98%           
=======================================
  Files          42       42           
  Lines        5704     5706    +2     
  Branches     1112     1113    +1     
=======================================
+ Hits         4391     4393    +2     
  Misses       1029     1029           
  Partials      284      284           
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/455?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/contrib/job.py](https://codecov.io/gh/glotzerlab/signac/pull/455/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvam9iLnB5) | `89.68% <100.00%> (+0.08%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/455?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/455?src=pr&el=footer). Last update [c1cf74e...f0404ba](https://codecov.io/gh/glotzerlab/signac/pull/455?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/changelog.txt b/changelog.txt
index f3df43b4..ff3be33f 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -18,7 +18,7 @@ Added
 Changed
 +++++++
 
- - Optimized job hash and equality checks (#442).
+ - Optimized job hash and equality checks (#442, #455).
  - Optimized ``H5Store`` initialization (#443).
  - State points are loaded lazily when ``Job`` is opened by id (#238, #239).
 
diff --git a/signac/contrib/job.py b/signac/contrib/job.py
index 2be6a58d..8cb5ed2d 100644
--- a/signac/contrib/job.py
+++ b/signac/contrib/job.py
@@ -151,9 +151,11 @@ class Job:
         return hash(self.id)
 
     def __eq__(self, other):
-        return (self.id == other.id) and (
-            os.path.realpath(self._wd) == os.path.realpath(other._wd)
-        )
+        if not isinstance(other, type(self)):
+            return NotImplemented
+        return self.id == other.id and os.path.realpath(
+            self.workspace()
+        ) == os.path.realpath(other.workspace())
 
     def __str__(self):
         """Return the job's id."""


Test patch:
diff --git a/tests/test_job.py b/tests/test_job.py
index 0855b301..ff7ed3b9 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -131,6 +131,39 @@ class TestJob(TestJobBase):
         job = self.project.open_job({"a": 0})
         assert str(job) == job.id
 
+    def test_eq(self):
+        job = self.project.open_job({"a": 0})
+
+        # Make sure that Jobs can only be equal to other Job instances.
+        class NonJob:
+            """Minimal class that cannot be compared with Job objects."""
+
+            def __init__(self, job):
+                self.id = job.id
+                self._workspace = job.workspace()
+
+        class JobSubclass(Job):
+            """Minimal subclass that can be compared with Job objects."""
+
+            def __init__(self, job):
+                self._id = job.id
+                self._workspace = job.workspace()
+
+            def workspace(self):
+                return self._workspace
+
+        non_job = NonJob(job)
+        assert job != non_job
+        assert non_job != job
+
+        sub_job = JobSubclass(job)
+        assert job == sub_job
+        assert sub_job == job
+
+        job2 = self.project.open_job({"a": 0})
+        assert job == job2
+        assert job2 == job
+
     def test_isfile(self):
         job = self.project.open_job({"a": 0})
         fn = "test.txt"