3 ms·

To be clear, a good init is not supposed to create a thread or do any execution that might not be instant of things like that. It would have been better to an

To be clear, a good init is not supposed to create a thread or do any execution that might not be instant of things like that. It would have been better to an addition start, run, exec method that does that kind of things. Even if for usage it is an inch more complicated to use with an additional call.

lmm 2y ago
> It would have been better to an addition start, run, exec method that does that kind of things. Even if for usage it is an inch more complicated to use with an additional call. That breaks RAII - you shouldn't give the user a way to create the object in an invalid state. Although if it's intended to be used as a context manager anyway then maybe doing it on context enter rather than on init would be nicer.
- greatgib 2y ago
  Doesn't necessarily have to be in an invalid state. You can have your object with a none value for the socket or a state like "not_started" and the different method will look at that to handle everything properly. Also, it was not supposed to be used in a context manager as there is just a close method but it is the caller that decided to wrap it in a context. But as what you suggest, indeed it would have been a great idea to add an __enter__ and do the starting there are as it would be the proper way.
crdrost 2y ago
I don't think it's totally crazy to, say, open a DB connection in __init__() even though that's not an instantaneous operation. That's not a hill I would die on, you can just say “open a connection and hand it to me as an argument,” but it just looks a little cleaner to me if the lifecycle of the connection is being handled inside this DB-operations class. (You can also defer creating the connection until it is actually used, or require an explicit call to connect it, but then you are also writing a bunch of boilerplate to make the type checker happy for cases where the class is having its methods called before it was properly connected.)
- mjr00 2y ago
  It's not totally crazy in that I see it all the time, but it's one of the two most common things I've found make Python code difficult to reason about.[0] After all, if you open a DB connection in __init__() -- how do you close it? This isn't C++ where we can tie that to a destructor. I've run into so many Python codebases that do this and have tons of unclosed connections as a result. A much cleaner way (IMO) to do this is use context managers that have explicit lifecycles, so something like this: @contextmanager def create_db_client(host: str, port: int) -> Generator[_DbClient, None, None]: try: connection = mydblib.connect(host, port) client = _DbClient(connection) yield client finally: connection.close() class _DbClient: def __init__(self, connection): self._connection = connection def do_thing_that_requires_connection(...): ... Which lets you write client code that looks like with create_db_client('localhost', 5432) as db_client: # port 3306 if you're a degenerate db_client.do_thing_that_requires_connection(...) This gives you type safety, connection safety, has minimal boilerplate for client code, and ensures the connection is created and disposed of properly. Obviously in larger codebases there's some more nuances, and you might want to implement a `typing.Protocol` for `_DbClient` that lets you pass it around, but IMO the general idea is much better than initializing a connection to a DB, ZeroMQ socket, gRPC client, etc in __init__. [0] The second is performing "heavy", potentially failing operations outside of functions and classes, which can cause failures when importing modules.
  - tayo42 2y ago
    I get the points everyone is making and they make sense, but sometimes you need persistent connections. Open and closing constantly like can cause issues
    - mjr00 2y ago
      There's nothing about the contextmanager approach that says you're open and closing any more or less frequently than a __init__ approach with a separate `close()` method. You're just statically ensuring 1) the close method gets called, and 2) database operations can only happen on an open connection. (or, notably, a connection that we expect to be open, as something external the system may have closed it in the meantime.) Besides, persistent connections are a bit orthogonal since you should be using a connection pool in practice, which most Python DB libs provide out of the box. In either case, the semantics are the same, open/close becomes lease from/return to pool.
eptcyka 2y ago
What if the object is in an invalid state unless the thread is started?
- greatgib 2y ago
  Nothing prevents you to have a valid "not_yet_started" state.
  - int_19h 2y ago
    This is basically equivalent to what null is for pointers, and has all the same problems.
  - rcxdude 2y ago
    No, but it does complicate things substantially. I don't understand why you would have a useless half-constructed state, especially in python where init isn't all that magical.
    - lgas 2y ago
      The benefits of making illegal states unrepresentable are just not widely understood outside of pure FP circles. I think it's hard for stuff like this to catch on in python in particular because everything is so accessible and mutable, it would be hard to talk about anything that provides the level of confidence that eg. ADTs in Haskell provide. And it's hard to understand why you would want to try to do something like that in a context like python unless you already have experience with it in a statically typed context.
  - eptcyka 2y ago
    And what would you do with this object after construction? Call start() on it immediately?
    - greatgib 2y ago
      Kind of yes or no, depend on your usage. But clearly would have worked well for the case of the automatic test if the start was the context
jhji7i77l 2y ago
> To be clear, a good init is not supposed to create a thread or do any execution that might not be instant of things like that. Maybe it indirectly ensures that only one thread is created per instantiation; there are better ways of achieving that though.

lmm 2y ago

> It would have been better to an addition start, run, exec method that does that kind of things. Even if for usage it is an inch more complicated to use with an additional call. That breaks RAII - you shouldn't give the user a way to create the object in an invalid state. Although if it's intended to be used as a context manager anyway then maybe doing it on context enter rather than on init would be nicer.

greatgib 2y ago
Doesn't necessarily have to be in an invalid state. You can have your object with a none value for the socket or a state like "not_started" and the different method will look at that to handle everything properly. Also, it was not supposed to be used in a context manager as there is just a close method but it is the caller that decided to wrap it in a context. But as what you suggest, indeed it would have been a great idea to add an __enter__ and do the starting there are as it would be the proper way.

crdrost 2y ago

I don't think it's totally crazy to, say, open a DB connection in __init__() even though that's not an instantaneous operation. That's not a hill I would die on, you can just say “open a connection and hand it to me as an argument,” but it just looks a little cleaner to me if the lifecycle of the connection is being handled inside this DB-operations class. (You can also defer creating the connection until it is actually used, or require an explicit call to connect it, but then you are also writing a bunch of boilerplate to make the type checker happy for cases where the class is having its methods called before it was properly connected.)

mjr00 2y ago
It's not totally crazy in that I see it all the time, but it's one of the two most common things I've found make Python code difficult to reason about.[0] After all, if you open a DB connection in __init__() -- how do you close it? This isn't C++ where we can tie that to a destructor. I've run into so many Python codebases that do this and have tons of unclosed connections as a result. A much cleaner way (IMO) to do this is use context managers that have explicit lifecycles, so something like this: @contextmanager def create_db_client(host: str, port: int) -> Generator[_DbClient, None, None]: try: connection = mydblib.connect(host, port) client = _DbClient(connection) yield client finally: connection.close() class _DbClient: def __init__(self, connection): self._connection = connection def do_thing_that_requires_connection(...): ... Which lets you write client code that looks like with create_db_client('localhost', 5432) as db_client: # port 3306 if you're a degenerate db_client.do_thing_that_requires_connection(...) This gives you type safety, connection safety, has minimal boilerplate for client code, and ensures the connection is created and disposed of properly. Obviously in larger codebases there's some more nuances, and you might want to implement a `typing.Protocol` for `_DbClient` that lets you pass it around, but IMO the general idea is much better than initializing a connection to a DB, ZeroMQ socket, gRPC client, etc in __init__. [0] The second is performing "heavy", potentially failing operations outside of functions and classes, which can cause failures when importing modules.
- tayo42 2y ago
  I get the points everyone is making and they make sense, but sometimes you need persistent connections. Open and closing constantly like can cause issues
  - mjr00 2y ago
    There's nothing about the contextmanager approach that says you're open and closing any more or less frequently than a __init__ approach with a separate `close()` method. You're just statically ensuring 1) the close method gets called, and 2) database operations can only happen on an open connection. (or, notably, a connection that we expect to be open, as something external the system may have closed it in the meantime.) Besides, persistent connections are a bit orthogonal since you should be using a connection pool in practice, which most Python DB libs provide out of the box. In either case, the semantics are the same, open/close becomes lease from/return to pool.

eptcyka 2y ago

What if the object is in an invalid state unless the thread is started?

greatgib 2y ago
Nothing prevents you to have a valid "not_yet_started" state.
- int_19h 2y ago
  This is basically equivalent to what null is for pointers, and has all the same problems.
- rcxdude 2y ago
  No, but it does complicate things substantially. I don't understand why you would have a useless half-constructed state, especially in python where init isn't all that magical.
  - lgas 2y ago
    The benefits of making illegal states unrepresentable are just not widely understood outside of pure FP circles. I think it's hard for stuff like this to catch on in python in particular because everything is so accessible and mutable, it would be hard to talk about anything that provides the level of confidence that eg. ADTs in Haskell provide. And it's hard to understand why you would want to try to do something like that in a context like python unless you already have experience with it in a statically typed context.
- eptcyka 2y ago
  And what would you do with this object after construction? Call start() on it immediately?
  - greatgib 2y ago
    Kind of yes or no, depend on your usage. But clearly would have worked well for the case of the automatic test if the start was the context

jhji7i77l 2y ago

> To be clear, a good init is not supposed to create a thread or do any execution that might not be instant of things like that. Maybe it indirectly ensures that only one thread is created per instantiation; there are better ways of achieving that though.