Skip to content

Conversation

@choksi81
Copy link
Contributor

On passing disallowed port numbers, time_interface was displaying misleading error in the form of raise TimeError("time_updatetime called before time_register_method!")
Also, there was an unreachable part of code raise TimeError('Error(s) in time_updatetime: ' + str(exception_list)), which has been fixed in this PR.

@vladimir-v-diaz
Copy link

@choksi81 Does this PR fix the issue you were having with the TCP relay unit test? Was it simply an invalid port number argument?

@choksi81
Copy link
Contributor Author

As it turns out, yes, that was the problem I was facing with TCP relay
unit-tests.

On 19 November 2014 13:35, Vladimir Diaz notifications@github.com wrote:

@choksi81 https://github.com/choksi81 Does this PR fix the issue you
were having with the TCP relay unit test? Was it simply an invalid port
number argument?


Reply to this email directly or view it on GitHub
#154 (comment)
.

@aaaaalbert
Copy link
Contributor

I ran the time unit tests against your patch, and they do pass. If any of the update_functions really raise errors, the TimeError plus exceptions_list is meaningful.

However, calling updatetime before time_register_method now results in this error:

TimeError('Error(s) in time_updatetime: []',)

(The for loop is not executed due to the empty TIME_IMP_DICT, and we directly jump into its else clause.)

We should thus raise an error before we attempt calling update_function if the TIME_IMP_DICT is empty to begin with, i.e. before the for loop.

  if not TIME_IMP_DICT:
    raise TimeError("time_updatetime called with an empty TIME_IMP_DICT. Use time_register_method to populate it!")

@vladimir-v-diaz
Copy link

9767003 now raises an exception if TIME_IMP_DICT is an empty dictionary. This PR LGTM.

@aaaaalbert May I merge some of these pull requests after reviewing them?

@aaaaalbert
Copy link
Contributor

Yes, please go ahead (assuming that the unit tests don't break).

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.

3 participants