Repository: glotzerlab/signac
Pull request #193 (2019-05-19 22:05:50)

Fix and improve repr of Project, Job, and JobsCursor.
Extend unit tests to check for identify of `eval(repr(x))` of affected
classes.

csadorf: Any other `__repr__()` functions we might want to revise?
vyasr: Collections would be nice. Also, I've personally found it awkward that `__str__` and `__repr__` are different for schemas, especially when I'm working in REPL environments and just want to see the schema by typing `project.detect_schema()`, but I don't know how everyone else feels about that. Maybe the thought was that it would be too verbose a default in those scenarios?
codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/193?src=pr&el=h1) Report
> Merging [#193](https://codecov.io/gh/glotzerlab/signac/pull/193?src=pr&el=desc) into [master](https://codecov.io/gh/glotzerlab/signac/commit/cbe589d155c505e3c892fca63db173e627e9c1b2?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
+ Coverage   65.03%   65.05%   +0.01%     
==========================================
  Files          37       37              
  Lines        5546     5548       +2     
==========================================
+ Hits         3607     3609       +2     
  Misses       1939     1939
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/193?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/contrib/job.py](https://codecov.io/gh/glotzerlab/signac/pull/193/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvam9iLnB5) | `90.47% <ø> (ø)` | :arrow_up: |
| [signac/contrib/project.py](https://codecov.io/gh/glotzerlab/signac/pull/193/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvcHJvamVjdC5weQ==) | `90.65% <100%> (+0.02%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/193?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/193?src=pr&el=footer). Last update [cbe589d...fc2b757](https://codecov.io/gh/glotzerlab/signac/pull/193?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

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

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

```diff
@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
+ Coverage   65.03%   65.05%   +0.01%     
==========================================
  Files          37       37              
  Lines        5546     5548       +2     
==========================================
+ Hits         3607     3609       +2     
  Misses       1939     1939
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/193?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/contrib/job.py](https://codecov.io/gh/glotzerlab/signac/pull/193/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvam9iLnB5) | `90.47% <ø> (ø)` | :arrow_up: |
| [signac/contrib/project.py](https://codecov.io/gh/glotzerlab/signac/pull/193/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvcHJvamVjdC5weQ==) | `90.65% <100%> (+0.02%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/193?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/193?src=pr&el=footer). Last update [cbe589d...fc2b757](https://codecov.io/gh/glotzerlab/signac/pull/193?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

csadorf: > Collections would be nice. Also, I've personally found it awkward that `__str__` and `__repr__` are different for schemas, especially when I'm working in REPL environments and just want to see the schema by typing `project.detect_schema()`, but I don't know how everyone else feels about that. Maybe the thought was that it would be too verbose a default in those scenarios?

Would you want the `__repr__` of Collections to be identical to `__str__`: `"<Collection file=<_io.TextIOWrapper name='collection.txt' encoding='utf-8'>>"` ?

I'd prefer the `ProjectSchema` representation to remain as compact as it is right now, but I'm open to suggestions.
vyasr: Hmm, is there a meaningful representation of the document index itself? I suppose the index for a collection is not as information rich as a project index, I was hoping to get something more like the output of `__str__` for a schema but I don't think that's possible. Perhaps an overview of file paths included or something, or even just mimicking the schema's `__repr__` method?

I just typically find myself writing `print(project.detect_schema())` any time I perform schema detection (or saving and then printing), and if others experience that too then I think we're maybe complicating the usage a little bit. If that's just me, though, then I'm fine keeping it as is if you prefer that.
csadorf: > Hmm, is there a meaningful representation of the document index itself? I suppose the index for a collection is not as information rich as a project index, I was hoping to get something more like the output of `__str__` for a schema but I don't think that's possible. Perhaps an overview of file paths included or something, or even just mimicking the schema's `__repr__` method?
> 
> I just typically find myself writing `print(project.detect_schema())` any time I perform schema detection (or saving and then printing), and if others experience that too then I think we're maybe complicating the usage a little bit. If that's just me, though, then I'm fine keeping it as is if you prefer that.

Can you create minimal demo implementations of what you would prefer? That would make the discussion a bit more concrete.

When you do, please follow guidelines outlined [here](https://stackoverflow.com/a/2626364), unless you disagree with them.

Patch:
diff --git a/changelog.txt b/changelog.txt
index f1542e26..749f38cc 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -26,6 +26,7 @@ next
  - Fix issue with incorrect detection of dict-like files managed with the ``DictManager`` class (e.g. ``job.stores``) (#203).
  - Fix issue causing a failure of the automatic conversion of valid key types (#168, #205).
  - Improve the 'dots in keys' error message to make it easier to fix related issues (#170, #205).
+ - Update the ``__repr__`` and ``__repr_html__`` implementations of the ``Project`, ``Job``, and ``JobsCursor`` classes (#193).
 
 
 [1.1.0] -- 2019-05-19
diff --git a/signac/contrib/job.py b/signac/contrib/job.py
index 54e1444a..fe632166 100644
--- a/signac/contrib/job.py
+++ b/signac/contrib/job.py
@@ -87,7 +87,7 @@ class Job(object):
 
     def __repr__(self):
         return "{}(project={}, statepoint={})".format(
-            self.__class__.__module__ + '.' + self.__class__.__name__,
+            self.__class__.__name__,
             repr(self._project), self._statepoint)
 
     def __eq__(self, other):
diff --git a/signac/contrib/project.py b/signac/contrib/project.py
index 95ad366a..51b0e2b8 100644
--- a/signac/contrib/project.py
+++ b/signac/contrib/project.py
@@ -160,13 +160,18 @@ class Project(object):
         return str(self.get_id())
 
     def __repr__(self):
-        return "{type}({{'project': '{id}', 'project_dir': '{rd}',"\
-               " 'workspace_dir': '{wd}'}})".format(
-                   type=self.__class__.__module__ + '.' + self.__class__.__name__,
-                   id=self.get_id(), rd=self.root_directory(), wd=self.workspace())
+        return "{type}.get_project('{root}')".format(
+                   type=self.__class__.__name__,
+                   root=self.root_directory())
 
     def _repr_html_(self):
-        return repr(self) + self.find_jobs()._repr_html_jobs()
+        return "<p>" + \
+            '<strong>Project:</strong> {}<br>'.format(self.get_id()) + \
+            "<strong>Root:</strong> {}<br>".format(self.root_directory()) + \
+            "<strong>Workspace:</strong> {}<br>".format(self.workspace()) + \
+            "<strong>Size:</strong> {}".format(len(self)) + \
+            "</p>" + \
+            self.find_jobs()._repr_html_jobs()
 
     def __eq__(self, other):
         return repr(self) == repr(other)
@@ -1597,6 +1602,10 @@ class JobsCursor(object):
         # next() method for this class.
         self._next_iter = None
 
+    def __eq__(self, other):
+        return self._project == other._project and self._filter == other._filter\
+            and self._doc_filter == other._doc_filter
+
     def __len__(self):
         # Highly performance critical code path!!
         if self._filter or self._doc_filter:
@@ -1789,12 +1798,11 @@ class JobsCursor(object):
             orient='index').infer_objects()
 
     def __repr__(self):
-        return "{type}({{'project': '{project}', 'filter': '{filter}',"\
-               " 'docfilter': '{doc_filter}'}})".format(
-                   type=self.__class__.__module__ + '.' + self.__class__.__name__,
-                   project=self._project,
-                   filter=self._filter,
-                   doc_filter=self._doc_filter)
+        return '{type}(project={project}, filter={filter}, doc_filter={doc_filter})'.format(
+                   type=self.__class__.__name__,
+                   project=repr(self._project),
+                   filter=repr(self._filter),
+                   doc_filter=repr(self._doc_filter))
 
     def _repr_html_jobs(self):
         html = ''


Test patch:
diff --git a/tests/test_job.py b/tests/test_job.py
index 04a4f931..6a8e0eb6 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -15,6 +15,8 @@ from contextlib import contextmanager
 
 import signac.contrib
 import signac.common.config
+from signac import Project
+from signac.contrib.job import Job
 from signac.common import six
 from signac.errors import DestinationExistsError
 from signac.errors import JobsCorruptedError
diff --git a/tests/test_project.py b/tests/test_project.py
index ec422ef9..e9ea86d3 100644
--- a/tests/test_project.py
+++ b/tests/test_project.py
@@ -19,6 +19,7 @@ from signac.contrib.schema import ProjectSchema
 from signac.contrib.errors import JobsCorruptedError
 from signac.contrib.errors import WorkspaceError
 from signac.contrib.errors import StatepointParsingError
+from signac.contrib.project import JobsCursor, Project
 
 from test_job import BaseJobTest
 
@@ -1338,7 +1339,7 @@ class ProjectRepresentationTest(BaseProjectTest):
             with self.subTest(type='str'):
                 str(self.project)
             with self.subTest(type='repr'):
-                repr(self.project)
+                self.assertEqual(eval(repr(self.project)), self.project)
             with self.subTest(type='html'):
                 for use_pandas in (True, False):
                     type(self.project)._use_pandas_for_html_repr = use_pandas
@@ -1353,7 +1354,8 @@ class ProjectRepresentationTest(BaseProjectTest):
                     with self.subTest(type='str'):
                         str(self.project.find_jobs(filter_))
                     with self.subTest(type='repr'):
-                        repr(self.project.find_jobs(filter_))
+                        q = self.project.find_jobs(filter_)
+                        self.assertEqual(eval(repr(q)), q)
                     with self.subTest(type='html'):
                         for use_pandas in (True, False):
                             type(self.project)._use_pandas_for_html_repr = use_pandas