Assert statement should be avoided in runtime code

[aaliddell]

When running Python with optimisations enabled, assert statements are skipped:

The current code generator emits no code for an assert statement when optimization is requested at compile time -- https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement

So assert statements are fine in test code or for library debugging, but should not be used in the library code for control flow or main type checks etc. Instead, if and raise should be used to give a proper error message to library users.

For example, the check for the passed proto instance being the correct type would be skipped and also presently raises a rather unhelpful AssertionError with no error context, where a TypeError would be more useful:

https://github.com/vmagamedov/grpclib/blob/5c1018b2b9446c8c3f105bcd23d0a5668a31c3c2/grpclib/encoding/proto.py#L18

# e.g. becomes
if not isinstance(message, message_type):
    raise TypeError('Incorrect type {} passed for proto, expected {}'.format(type(message), message_type))

[vmagamedov]

Thanks! Such TypeError is more user-friendly.

[aaliddell]

:+1:

There are still a number of other asserts in the non-testing library code besides just that example above though. Some probably fit in the 'debugging' camp, but it might be worth checking them.

A grep for assert in the grpclib directory gives this:

grpclib/events.py:21:        assert len(kwargs) == len(self.__slots__), self.__slots__
grpclib/events.py:98:                assert (isinstance(dispatches, type)
grpclib/events.py:100:                assert dispatches not in dispatch_methods, dispatches
grpclib/client.py:490:                assert exc_val is not None
grpclib/client.py:744:            assert reply is not None
grpclib/client.py:809:            assert reply is not None
grpclib/protocol.py:90:        assert size >= 0, 'Size can not be negative'
grpclib/protocol.py:126:        assert chunks_size == size
grpclib/protocol.py:168:        assert value is None or value >= 0, value
grpclib/protocol.py:277:        assert self.headers is not None
grpclib/protocol.py:286:        assert self.trailers is not None
grpclib/protocol.py:296:        assert self.id is None, self.id
grpclib/protocol.py:336:        assert self.id is not None
grpclib/protocol.py:473:        assert stream.id is not None
grpclib/protocol.py:478:        assert stream.id is not None
grpclib/protocol.py:482:            assert stream.id is not None
grpclib/stream.py:30:    assert len(message_bin) == message_len, \
grpclib/testing.py:62:            assert response.message == 'Hello, Dr. Strange!'
grpclib/testing.py:97:        assert self._channel._protocol is not None
grpclib/utils.py:64:        assert task
grpclib/utils.py:121:    assert method_name is not None
grpclib/health/check.py:146:            assert self._poll_task is not None
grpclib/health/service.py:91:        assert request is not None
grpclib/health/service.py:111:        assert request is not None

[vmagamedov]

Most of these asserts were added specifically for mypy, to convert Optional[thing] into thing. Checked all current asserts, seems like all of them are properly placed.

[aaliddell]

Ok, thanks!