Skip to content
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

TypeError on method call with no inputArguments. Added case to handle this. #964

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

craigh92
Copy link

Closes Issue 963

@@ -128,7 +128,7 @@ def test_subscription_count_list(self):
val = copy(val) # we do not want to modify object in our db, we need a copy in order to generate event
val.append(i)
var.set_value(copy(val))
time.sleep(0.2) # let last event arrive
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was too short and causing the test to occasionally fail. I have increased it to 0.3.

@@ -463,6 +463,8 @@ def _call(self, method):
res.StatusCode = ua.StatusCode(ua.StatusCodes.BadNothingToDo)
else:
try:
if method.InputArguments is None:
method.InputArguments = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these lines will never fail. No need to have them under try/except

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines will never fail, but the actual method call could. I have put this check in the same scope as the method call for readability. Would you prefer it outside of the try statement?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I just looked at that code. In fact we should only have try/except around the node.call line. Can you move everything else out?

@oroulet
Copy link
Member

oroulet commented Feb 18, 2020

Probably fine, but canyou add a test for that new behaviour? I am curious to know when it happend. I thought this was well supported

@oroulet
Copy link
Member

oroulet commented Mar 9, 2020

@craigh92 any news?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants