-
Notifications
You must be signed in to change notification settings - Fork 22
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
more robust validation of return value from destfnscript #1085
Conversation
Test Results242 tests 234 ✅ 19s ⏱️ Results for commit 7efabbf. ♻️ This comment has been updated with latest results. |
here is the old "error message":
and the instance crashes. |
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.
There's a change in behaviour. Before the instance would crash and the file wouldn't get sent. Now if the destfnscript crashes, the file gets sent with the original name.
Is that okay? Would it be better to reject the message?
I dunno what is best to do... if it crashed then it would be obvious that something is wrong... with a destfn script... I mean it will drop some files in the flow... I guess missing is easier to understand than named wrong? ok. |
OK... so the problem is that we aren't in mainline flow code... we are in the routine sundew_getDestInfos whose return value is the new file name... and that is called from updateFieldsAccepted... which has no return value... |
That could work. I'm not saying this is better, just an alternative option: sundew_getDestInfos is called from updateFieldsAccepted, and that is called in the filter method. The try/except could be moved into filter, and the messages could be rejected there? |
somehow this is already in development branch... I'm confused... merged by some other merge... anyways I will try to make it return None, and have the various callers deal with that to do something smart. |
OK... it returns None as the file name in the error case, and now the main routines do_download and do_send check for empty new_file and new_directory fields and reject them. Good? |
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.
yes, that's great. thanks
saw how inscrutable the error message was for Daniel, when his destfnscript had a problem.
wrapped it in a try/except and added minimal validation of the return value... give people a hint.