From 26f6fee4356e2d0c655a02fb48ecc567dad3ca4f Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Thu, 6 May 2021 18:24:08 -0700 Subject: [PATCH 1/8] falsifying test --- testsuite/MDAnalysisTests/analysis/test_base.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/testsuite/MDAnalysisTests/analysis/test_base.py b/testsuite/MDAnalysisTests/analysis/test_base.py index 71ea1aea3e3..8ff58daffaa 100644 --- a/testsuite/MDAnalysisTests/analysis/test_base.py +++ b/testsuite/MDAnalysisTests/analysis/test_base.py @@ -76,6 +76,14 @@ def test_weird_key(self, results, key): with pytest.raises(ValueError, match=msg): results[key] = None + @pytest.mark.xfail(reason="conflict in key validation", strict=True) + def test_setattr_modify_item(self, results): + setattr(results, "myattr", 0) + assert results.myattr == 0 + results["myattr"] = 3 + assert results.myattr == 3 + + class FrameAnalysis(base.AnalysisBase): """Just grabs frame numbers of frames it goes over""" From 44803ebe6bb2426d95f22aaf4e54846e576cf922 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Thu, 6 May 2021 19:03:39 -0700 Subject: [PATCH 2/8] setattr == setitem --- package/MDAnalysis/analysis/base.py | 28 +++++++++---------- .../MDAnalysisTests/analysis/test_base.py | 11 ++++---- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/package/MDAnalysis/analysis/base.py b/package/MDAnalysis/analysis/base.py index c7619203581..f71ff1236e6 100644 --- a/package/MDAnalysis/analysis/base.py +++ b/package/MDAnalysis/analysis/base.py @@ -31,6 +31,7 @@ import inspect import logging import itertools +import functools import numpy as np from MDAnalysis import coordinates @@ -85,7 +86,7 @@ class in `scikit-learn`_. .. versionadded:: 2.0.0 """ def _validate_key(self, key): - if key in dir(UserDict) or (key == "data" and self._dict_frozen): + if key in dir(UserDict) or key == "data": raise AttributeError(f"'{key}' is a protected dictionary " "attribute") elif isinstance(key, str) and not key.isidentifier(): @@ -94,28 +95,25 @@ def _validate_key(self, key): def __init__(self, **kwargs): if "data" in kwargs.keys(): raise AttributeError(f"'data' is a protected dictionary attribute") - - self._dict_frozen = False - for key in kwargs: - self._validate_key(key) - super().__init__(**kwargs) - self._dict_frozen = True + self.__dict__["data"] = {} + self.update(kwargs) def __setitem__(self, key, item): self._validate_key(key) super().__setitem__(key, item) - def __setattr__(self, attr, value): - self._validate_key(attr) - super().__setattr__(attr, value) - - # attribute available as key - if self._dict_frozen and attr != "_dict_frozen": - super().__setitem__(attr, value) + __setattr__ = __setitem__ def __getattr__(self, attr): try: - return self.data[attr] + return self[attr] + except KeyError as err: + raise AttributeError("'Results' object has no " + f"attribute '{attr}'") from err + + def __delattr__(self, attr): + try: + del self[attr] except KeyError as err: raise AttributeError("'Results' object has no " f"attribute '{attr}'") from err diff --git a/testsuite/MDAnalysisTests/analysis/test_base.py b/testsuite/MDAnalysisTests/analysis/test_base.py index 8ff58daffaa..9ad07df9334 100644 --- a/testsuite/MDAnalysisTests/analysis/test_base.py +++ b/testsuite/MDAnalysisTests/analysis/test_base.py @@ -45,23 +45,24 @@ def test_get(self, results): assert results.a == results["a"] == 1 def test_no_attr(self, results): - with pytest.raises(AttributeError): + msg = "'Results' object has no attribute 'c'" + with pytest.raises(AttributeError, match=msg): results.c def test_set_attr(self, results): value = [1, 2, 3, 4] results.c = value - assert results.c == results["c"] == value + assert results.c is results["c"] is value def test_set_key(self, results): value = [1, 2, 3, 4] results["c"] = value - assert results.c == results["c"] == value + assert results.c is results["c"] is value @pytest.mark.parametrize('key', dir(UserDict) + ["data"]) def test_existing_dict_attr(self, results, key): msg = f"'{key}' is a protected dictionary attribute" - with pytest.raises(AttributeError, match=key): + with pytest.raises(AttributeError, match=msg): results[key] = None @pytest.mark.parametrize('key', dir(UserDict) + ["data"]) @@ -76,7 +77,6 @@ def test_weird_key(self, results, key): with pytest.raises(ValueError, match=msg): results[key] = None - @pytest.mark.xfail(reason="conflict in key validation", strict=True) def test_setattr_modify_item(self, results): setattr(results, "myattr", 0) assert results.myattr == 0 @@ -84,7 +84,6 @@ def test_setattr_modify_item(self, results): assert results.myattr == 3 - class FrameAnalysis(base.AnalysisBase): """Just grabs frame numbers of frames it goes over""" From fd70a3172ddfe1326b72e872d119390f7681e674 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Thu, 6 May 2021 19:08:03 -0700 Subject: [PATCH 3/8] make tests more rigorous --- package/MDAnalysis/analysis/base.py | 7 ------ .../MDAnalysisTests/analysis/test_base.py | 24 +++++++++++++++---- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/package/MDAnalysis/analysis/base.py b/package/MDAnalysis/analysis/base.py index f71ff1236e6..5005bb76b36 100644 --- a/package/MDAnalysis/analysis/base.py +++ b/package/MDAnalysis/analysis/base.py @@ -111,13 +111,6 @@ def __getattr__(self, attr): raise AttributeError("'Results' object has no " f"attribute '{attr}'") from err - def __delattr__(self, attr): - try: - del self[attr] - except KeyError as err: - raise AttributeError("'Results' object has no " - f"attribute '{attr}'") from err - class AnalysisBase(object): r"""Base class for defining multi-frame analysis diff --git a/testsuite/MDAnalysisTests/analysis/test_base.py b/testsuite/MDAnalysisTests/analysis/test_base.py index 9ad07df9334..d450bc4a9c0 100644 --- a/testsuite/MDAnalysisTests/analysis/test_base.py +++ b/testsuite/MDAnalysisTests/analysis/test_base.py @@ -78,10 +78,26 @@ def test_weird_key(self, results, key): results[key] = None def test_setattr_modify_item(self, results): - setattr(results, "myattr", 0) - assert results.myattr == 0 - results["myattr"] = 3 - assert results.myattr == 3 + mylist = [1, 2] + mylist2 = [3, 4] + results.myattr = mylist + assert results.myattr is mylist + results["myattr"] = mylist2 + assert results.myattr is mylist2 + mylist2.pop(0) + assert len(results.myattr) == 1 + assert results.myattr is mylist2 + + def test_setitem_modify_item(self, results): + mylist = [1, 2] + mylist2 = [3, 4] + results["myattr"] = mylist + assert results.myattr is mylist + results.myattr = mylist2 + assert results.myattr is mylist2 + mylist2.pop(0) + assert len(results["myattr"]) == 1 + assert results["myattr"] is mylist2 class FrameAnalysis(base.AnalysisBase): From 867f37b700bb0d3869274267a9cd774a9dc1632d Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Thu, 6 May 2021 19:08:36 -0700 Subject: [PATCH 4/8] added delattr for completions sake --- package/MDAnalysis/analysis/base.py | 7 +++++++ .../MDAnalysisTests/analysis/test_base.py | 20 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/package/MDAnalysis/analysis/base.py b/package/MDAnalysis/analysis/base.py index 5005bb76b36..f71ff1236e6 100644 --- a/package/MDAnalysis/analysis/base.py +++ b/package/MDAnalysis/analysis/base.py @@ -111,6 +111,13 @@ def __getattr__(self, attr): raise AttributeError("'Results' object has no " f"attribute '{attr}'") from err + def __delattr__(self, attr): + try: + del self[attr] + except KeyError as err: + raise AttributeError("'Results' object has no " + f"attribute '{attr}'") from err + class AnalysisBase(object): r"""Base class for defining multi-frame analysis diff --git a/testsuite/MDAnalysisTests/analysis/test_base.py b/testsuite/MDAnalysisTests/analysis/test_base.py index d450bc4a9c0..d8ccd28e55e 100644 --- a/testsuite/MDAnalysisTests/analysis/test_base.py +++ b/testsuite/MDAnalysisTests/analysis/test_base.py @@ -99,6 +99,26 @@ def test_setitem_modify_item(self, results): assert len(results["myattr"]) == 1 assert results["myattr"] is mylist2 + def test_delattr(self, results): + assert hasattr(results, "a") + delattr(results, "a") + assert not hasattr(results, "a") + + def test_pop(self, results): + assert hasattr(results, "a") + results.pop("a") + assert not hasattr(results, "a") + + def test_update(self, results): + assert not hasattr(results, "spudda") + results.update({"spudda": "fett"}) + assert results.spudda == "fett" + + def test_update_data_fail(self, results): + msg = f"'data' is a protected dictionary attribute" + with pytest.raises(AttributeError, match=msg): + results.update({"data": 0}) + class FrameAnalysis(base.AnalysisBase): """Just grabs frame numbers of frames it goes over""" From beb6e27d69af30fba90f0b161427a5b93e9f5359 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Thu, 6 May 2021 19:12:35 -0700 Subject: [PATCH 5/8] simplify validation --- package/MDAnalysis/analysis/base.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/package/MDAnalysis/analysis/base.py b/package/MDAnalysis/analysis/base.py index f71ff1236e6..b14df02ea72 100644 --- a/package/MDAnalysis/analysis/base.py +++ b/package/MDAnalysis/analysis/base.py @@ -31,7 +31,6 @@ import inspect import logging import itertools -import functools import numpy as np from MDAnalysis import coordinates @@ -85,8 +84,11 @@ class in `scikit-learn`_. .. versionadded:: 2.0.0 """ + + data = None + def _validate_key(self, key): - if key in dir(UserDict) or key == "data": + if key in dir(self): raise AttributeError(f"'{key}' is a protected dictionary " "attribute") elif isinstance(key, str) and not key.isidentifier(): From 54c69f312405f61f3c220814c74eddb3a2754df2 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 7 May 2021 07:37:06 -0700 Subject: [PATCH 6/8] add more initialization arguments --- package/MDAnalysis/analysis/base.py | 3 ++- testsuite/MDAnalysisTests/analysis/test_base.py | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/package/MDAnalysis/analysis/base.py b/package/MDAnalysis/analysis/base.py index b14df02ea72..6307527e8bd 100644 --- a/package/MDAnalysis/analysis/base.py +++ b/package/MDAnalysis/analysis/base.py @@ -94,7 +94,8 @@ def _validate_key(self, key): elif isinstance(key, str) and not key.isidentifier(): raise ValueError(f"'{key}' is not a valid attribute") - def __init__(self, **kwargs): + def __init__(self, *args, **kwargs): + kwargs = dict(*args, **kwargs) if "data" in kwargs.keys(): raise AttributeError(f"'data' is a protected dictionary attribute") self.__dict__["data"] = {} diff --git a/testsuite/MDAnalysisTests/analysis/test_base.py b/testsuite/MDAnalysisTests/analysis/test_base.py index d8ccd28e55e..66171849a14 100644 --- a/testsuite/MDAnalysisTests/analysis/test_base.py +++ b/testsuite/MDAnalysisTests/analysis/test_base.py @@ -119,6 +119,18 @@ def test_update_data_fail(self, results): with pytest.raises(AttributeError, match=msg): results.update({"data": 0}) + @pytest.mark.parametrize("args, kwargs, length", [ + (({"darth": "tater"},), {}, 1), + ([], {"darth": "tater"}, 1), + (({"darth": "tater"},), {"yam": "solo"}, 2), + (({"darth": "tater"},), {"darth": "vader"}, 1), + ]) + def test_initialize_arguments(self, args, kwargs, length): + results = base.Results(*args, **kwargs) + ref = dict(*args, **kwargs) + assert ref == results + assert len(results) == length + class FrameAnalysis(base.AnalysisBase): """Just grabs frame numbers of frames it goes over""" From 5ed868afb343bf43be1f4e501f164f84068144ab Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 7 May 2021 11:35:28 -0700 Subject: [PATCH 7/8] test different instances --- package/MDAnalysis/analysis/base.py | 2 -- testsuite/MDAnalysisTests/analysis/test_base.py | 4 ++++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/package/MDAnalysis/analysis/base.py b/package/MDAnalysis/analysis/base.py index 6307527e8bd..f38a24890eb 100644 --- a/package/MDAnalysis/analysis/base.py +++ b/package/MDAnalysis/analysis/base.py @@ -85,8 +85,6 @@ class in `scikit-learn`_. .. versionadded:: 2.0.0 """ - data = None - def _validate_key(self, key): if key in dir(self): raise AttributeError(f"'{key}' is a protected dictionary " diff --git a/testsuite/MDAnalysisTests/analysis/test_base.py b/testsuite/MDAnalysisTests/analysis/test_base.py index 66171849a14..3f69b40ba69 100644 --- a/testsuite/MDAnalysisTests/analysis/test_base.py +++ b/testsuite/MDAnalysisTests/analysis/test_base.py @@ -131,6 +131,10 @@ def test_initialize_arguments(self, args, kwargs, length): assert ref == results assert len(results) == length + def test_different_instances(self, results): + new_results = base.Results(darth="tater") + assert new_results.data is not results.data + class FrameAnalysis(base.AnalysisBase): """Just grabs frame numbers of frames it goes over""" From dc7c0f1c9a2f368ca2f5597294478f9673ef7c1f Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 7 May 2021 12:48:49 -0700 Subject: [PATCH 8/8] another test for codecov --- testsuite/MDAnalysisTests/analysis/test_base.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/testsuite/MDAnalysisTests/analysis/test_base.py b/testsuite/MDAnalysisTests/analysis/test_base.py index 3f69b40ba69..9c4995e4c07 100644 --- a/testsuite/MDAnalysisTests/analysis/test_base.py +++ b/testsuite/MDAnalysisTests/analysis/test_base.py @@ -104,6 +104,12 @@ def test_delattr(self, results): delattr(results, "a") assert not hasattr(results, "a") + def test_missing_delattr(self, results): + assert not hasattr(results, "d") + msg = "'Results' object has no attribute 'd'" + with pytest.raises(AttributeError, match=msg): + delattr(results, "d") + def test_pop(self, results): assert hasattr(results, "a") results.pop("a")