Use @property to prevent direct modification of all_arrays
#196
Conversation
|
|
||
| @property | ||
| def all_arrays(self): | ||
| return self._all_arrays |
There was a problem hiding this comment.
A private property _all_arrays is used internally.
| def all_arrays(self): | ||
| return self._all_arrays | ||
|
|
||
| @all_arrays.setter |
There was a problem hiding this comment.
when the user attempts to modify all_array, throw an exception. I've thought it might be better to use exception rather than warning since our _set_xarrays is doing some heavy lifting work and we want our users to use the way we want them to do?
| compare_dicts(actual_dict, expected) | ||
|
|
||
|
|
||
| def test_all_array_setter(): |
There was a problem hiding this comment.
Test for throwing an exception when the user attempts to modify all_arrays directly.
bobleesj
left a comment
bobleesj
left a comment
There was a problem hiding this comment.
@sbillinge Ready for review!
bobleesj
changed the title
@property to prevent direction modification of all_arrays@property to prevent direct modification of all_arrays
sbillinge
left a comment
sbillinge
left a comment
There was a problem hiding this comment.
pls see inline. I mostly just reviewed the tests.
tests/test_diffraction_objects.py
Outdated
| actualdo = DiffractionObject(**inputs) | ||
| compare_dicts(actualdo.__dict__, expected) | ||
| actual_do = DiffractionObject(**inputs) | ||
| actual_dict = { |
There was a problem hiding this comment.
we normally do this work using actual_do.__dict__ so the test will fail if someone adds a new attribute to DiffractionObject
tests/test_diffraction_objects.py
Outdated
| actual_do = DiffractionObject() | ||
|
|
||
| # Attempt to directly modify the property | ||
| with pytest.raises(AttributeError, match="Direct modification of 'all_arrays' is not allowed."): |
There was a problem hiding this comment.
let's follow our favorite pattern. "what went wrong. How to fix it". The first part is done (though I would add the word "attribute" before 'all_arrays') but not the second? I think we want to point them towards using insert_diffraction_object or sthg, but give it some thought.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #196 +/- ##
==========================================
- Coverage 96.53% 93.97% -2.56%
==========================================
Files 8 8
Lines 289 299 +10
==========================================
+ Hits 279 281 +2
- Misses 10 18 +8
|
| @pytest.mark.parametrize("inputs, expected", tc_params) | ||
| def test_constructor(inputs, expected): | ||
| actualdo = DiffractionObject(**inputs) | ||
| compare_dicts(actualdo.__dict__, expected) |
There was a problem hiding this comment.
Can we use a library DeepDiff to compare two dicts instead? We've manually defined compare_dicts and dicts_equal within test_diffraction_objects.py which can be replaced using DeepDiff across all .py files.
| assert diff == {} | ||
|
|
||
|
|
||
| def test_all_array_getter(): |
There was a problem hiding this comment.
Test getter for all_arrays
|
@sbillinge ready for review again |
2 participants
Closes #194