-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apply overrides for data_input_dict in Node #91
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple points here - if you're happy with my suggestions please just double check the tests still run and there's no typos before merging
Co-authored-by: barneydobson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed the load error I think
Co-authored-by: barneydobson <[email protected]>
Co-authored-by: barneydobson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of areas of improvement.
Co-authored-by: barneydobson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ok, but it needs a couple of tweaks and extra tests.
@@ -417,6 +419,27 @@ def test_data_read(self): | |||
|
|||
self.assertEqual(15, node.get_data_input("temperature")) | |||
|
|||
def test_data_overrides(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only test the first branch of the conditional statement. You need another test for the case of not content
and another one for the else
case, which is an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added commit 91f198d
Co-authored-by: Diego Alonso Álvarez <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Please note these changes do no apply to
Surface
and its superclasses as they are based on theDecayTank
, which does not havedata_input_dict
attribute. The overrides fordata_input_dict
inSurface
are addressed in issue #76.