Add gettable id property to DiffractionObject
#216
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #216 +/- ##
==========================================
+ Coverage 99.69% 99.70% +0.01%
==========================================
Files 8 8
Lines 325 340 +15
==========================================
+ Hits 324 339 +15
Misses 1 1
|
| actual_do = DiffractionObject(**inputs) | ||
| diff = DeepDiff(actual_do.__dict__, expected, ignore_order=True, significant_digits=13) | ||
| actual = DiffractionObject(**inputs).__dict__ | ||
| diff = DeepDiff(actual, expected, ignore_order=True, significant_digits=13, exclude_paths="root['_id']") |
There was a problem hiding this comment.
for testing constructor, _id is ignored since it's hard to dynamically compare its uuid value.
There was a problem hiding this comment.
is that ok? not sure if there is a better way for this.
There was a problem hiding this comment.
In the past I have mocked that attribute. This here seems less work. If we have tests that test the id setting and getting, we can safely ignore it in other tests that are testing other things, such as this one, think
There was a problem hiding this comment.
Noted - yes is setter/getter test funcs provided below.
| @@ -384,7 +403,7 @@ def test_input_xtype_getter(): | |||
| assert do.input_xtype == "tth" | |||
|
|
|||
|
|
|||
| def test_input_xtype_setter(): | |||
| def test_input_xtype_setter_error(): | |||
There was a problem hiding this comment.
just enhancing the function name for clarify
There was a problem hiding this comment.
@sbillinge ready for review with in-line comments added for your feedback.
|
|
||
|
|
||
| def test_id_getter_with_mock(mocker): | ||
| mocker.patch.object(DiffractionObject, "id", new_callable=lambda: UUID("d67b19c6-3016-439f-81f7-cf20a04bee87")) |
There was a problem hiding this comment.
Since id is used with @property, the following doesn't apply:
mocker.patch.object(DiffractionObject, 'id', return_value="d67b19c6-3016-439f-81f7-cf20a04bee87")
Hence we use the new_callable parameter.
There was a problem hiding this comment.
I see, nicely done. I didn't know about that.
|
@sbillinge ready for review - codecov is causing an issue although I re-ran the CI with an empty commit. 
|
|
|
||
|
|
||
| def test_id_getter_with_mock(mocker): | ||
| mocker.patch.object(DiffractionObject, "id", new_callable=lambda: UUID("d67b19c6-3016-439f-81f7-cf20a04bee87")) |
There was a problem hiding this comment.
I see, nicely done. I didn't know about that.
|
@bobleesj @yucongalicechen I merged this but now I am having second thoughts. Do we want the id associated with the data rather than the DO itself? In that case we should mint it in More discussion: this would go away if we don't allow an empty instance where data are loaded later. I started leaning that way because I couldn't think of a UC where we would really need that, though one would be if someone wants to load directly from a file to the DO. We could also allow that as an option in the constructor too. Can you have a think about it? If we do mint the id on the load data function, all of our setters will have to have it insterted too. Summary: maybe the cleanest is to mint it in the constructor as we do now but remove the ability to load data outside of the constructor (and make that method private). But aloe an option to instantiate from file by giving a file path instead of a set of arrays. What do you think? I think I like that... |
|
since this PR has been merged, I created an issue #224 to continue our discussions |
Closes #213
Currently in Draft mode, since I am waiting for #215 to be merged, which modifies
input_scattering_quantitytoinput_datain the same file.As a starting point, this PR just adds
uuidattribute toDiffracitonObject.From the Slack chat, we had a greater vision. If we decide to work on it, should you make an issue for that? @sbillinge