Repository: glotzerlab/signac

Issue:
Calling `_SyncedDict.values()` gives `dict` objects
<!-- Please replace the text in the individual sections below. -->

### Description

Calling standard `MutableMapping` methods (`keys`, `values`, etc) on `_SyncedAttrDict` objects modifies the underlying dictionary in place. This behavior is unexpected, such an operation should only affect the returned value.

### To reproduce

```
>> import signac
>> project = signac.init_project('test')
>> job = project.open_job({'a': {'b': 'c'}}).init()
>> print(type(job.sp.a))
<class 'signac.core.attrdict.SyncedAttrDict'>
>> job.sp.values()
>> print(type(job.sp.a))
<class 'dict'>
```

There is an open question of whether `job.sp.values()` should return nested dictionaries as `SyncedDict` objects or normal `dict` objects. I prefer that `SyncedDict` objects be what it returns so that it's still going to sync, as does @bdice, but that is a separate question. Either way, calling values in the manner in this PR should not modify the object in place.

### System configuration

Linux-4.19.61-1-lts-x86_64-with-arch
3.7.4 (default, Oct  4 2019, 06:57:26)
[GCC 9.2.0]
1.2.0



Discussion:
bdice: @vyasr Since the names of our CI checks changed with #266, you may need to merge `master` into this branch to make the required checks pass. I'm fine with you overriding the required checks if you want, since this PR is fairly small and doesn't touch anything altered in #266.
codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/269?src=pr&el=h1) Report
> Merging [#269](https://codecov.io/gh/glotzerlab/signac/pull/269?src=pr&el=desc) into [master](https://codecov.io/gh/glotzerlab/signac/commit/e5cd6726d1d6a1f0ea0f61d21b03dbaa744ab66a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

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

```diff
@@           Coverage Diff           @@
##           master     #269   +/-   ##
=======================================
  Coverage   65.42%   65.42%           
=======================================
  Files          42       42           
  Lines        5697     5697           
=======================================
  Hits         3727     3727           
  Misses       1970     1970
```



------

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

csadorf: @vyasr Can you update the name of this PR to be a bit more descriptive?
vyasr: Yes, sorry it was a single commit so it just autopopulated the title and I didn't change it.

Patch:
diff --git a/changelog.txt b/changelog.txt
index d099ad1a..bbd04bb9 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -19,6 +19,7 @@ Fixed
 +++++
 
  - Fixed issues on Windows with ``H5Store``, project import/export, and operations that move files (#264, #266).
+ - Calling ``items`` or ``values`` on _SyncedDict objects does not mutate nested dictionaries (#234, #269).
 
 
 [1.3.0] -- 2019-12-20
diff --git a/signac/core/synceddict.py b/signac/core/synceddict.py
index e3eb1393..48c74555 100644
--- a/signac/core/synceddict.py
+++ b/signac/core/synceddict.py
@@ -265,11 +265,11 @@ class _SyncedDict(MutableMapping):
 
     def values(self):
         self._synced_load()
-        return self._convert_to_dict(self._data).values()
+        return self._convert_to_dict(self).values()
 
     def items(self):
         self._synced_load()
-        return self._convert_to_dict(self._data).items()
+        return self._convert_to_dict(self).items()
 
     def __repr__(self):
         return repr(self())
@@ -279,7 +279,7 @@ class _SyncedDict(MutableMapping):
 
     def _as_dict(self):
         with self._suspend_sync():
-            return self._convert_to_dict(self._data.copy())
+            return self._convert_to_dict(self)
 
     def __call__(self):
         self._synced_load()


Test patch:
diff --git a/tests/test_synced_attrdict.py b/tests/test_synced_attrdict.py
index 503c758d..6509e363 100644
--- a/tests/test_synced_attrdict.py
+++ b/tests/test_synced_attrdict.py
@@ -508,6 +508,18 @@ class SyncedAttrDictTest(unittest.TestCase):
         self.assertEqual(len(sad), 1)
         self.assert_only_read()
 
+    def test_nested_types_dict_conversion(self):
+        """Ensure that calling methods like items and values does not
+        change the type of nested dictionaries."""
+        sad = self.get_sad({'a': {'b': 1}})
+        assert type(sad['a']) is SAD
+        sad.items()
+        assert type(sad['a']) is SAD
+        sad.values()
+        assert type(sad['a']) is SAD
+        sad._as_dict()
+        assert type(sad['a']) is SAD
+
 
 if __name__ == '__main__':
     unittest.main()