Uh oh!
There was an error while loading. Please reload this page.
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?
Uh oh!
There was an error while loading. Please reload this page.
| 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
There was a problem hiding this comment.
@sbillinge Ready for review!
@property to prevent direction modification of all_arrays@property to prevent direct modification of all_arrays
sbillinge
left a comment
There was a problem hiding this comment.
pls see inline. I mostly just reviewed the tests.
| 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
| 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
bobleesj
commented
Dec 5, 2024
@sbillinge ready for review again |
Closes#194