Way less globals #37
Way less globals #37
Conversation
…e (also remove metaclass). Some other renaming, small fixes ...
|
I think I've addressed all your comments. About the merge, my plan is to re-integrate all the small changes from master and then do a "merge --strategy=ours" (discard everything from master). It's way easier that way. After that I'm going to rebase the remaining PRs on top this. |
|
I can squash everything but then we lose all your comments. Ideally you'd look here https://github.com/ionelmc/python-manhole/pull/37/files (it's an equivalent of a squash). |
|
Ok, https://github.com/ionelmc/python-manhole/pull/37/files does help. |
|
Anway, what I do when I want to keep versions, is to create a new branch for each version, for example, see https://github.com/nirs/sanlock - branchs python-events, python-events-1, ... |
| @@ -156,8 +135,8 @@ class Manhole(_ORIGINAL_THREAD): | |||
| daemon_connection (bool): The connection thread is daemonic (dies on app exit). Default: ``False``. | |||
| """ | |||
|
|
|||
| def __init__(self, sigmask, start_timeout, bind_delay=None, locals=None, daemon_connection=False): | |||
| super(Manhole, self).__init__() | |||
| def __init__(self, get_socket, sigmask, start_timeout, bind_delay=None, locals=None, daemon_connection=False): | |||
There was a problem hiding this comment.
Why do we have to have get_socket as an argument? Since get_socket is static now, it should simply be a module function - any reason to keep it elsewhere?
Or simply use _MANHOLE.get_socket()
There was a problem hiding this comment.
Because it's not assigned to _MANHOLE when get_socket needs to be called. This can be removed if we change how the assignment is done ( 4503f8a#commitcomment-8307134 )
There was a problem hiding this comment.
Right - doing nothing in init and moving all side effects to Manhole.install() will fix that.
|
That makes sense if you want to maintain multiple versions at once. You really want version branches? |
| else: | ||
| self.destination.write(full_message) | ||
| except: # pylint: disable=W0702 | ||
| pass |
There was a problem hiding this comment.
Empty line or two after the class will make it easier to read and pep8 happier.
|
Reviewing the the pull request diffs is fine for me. |
…he single-instance checks critical section is done).
It is not documneted and it does not make sense that manhole will provide this interface.
| if _SHOULD_RESTART: | ||
| _INST.start() | ||
| _LOG.configure(verbose, verbose_destination) | ||
| _MANHOLE.configure(**kwargs) # Threads might be started here |
There was a problem hiding this comment.
This does not solve the dependencies issue.
I think we need this api for the manhole:
__init__- keep configuration, no side effectsinstall- patching fork, register signal handlersactivate- bind socket, start thread
In oneshot mode, the mahole is installed but never activated
in activate_on mode, the manhole is installed but activate only when receiving the activation signal
in default mode, the manhole is installed and activated immediately
So when manhole thread run, _MANHOLE.get_socket() is available.
There was a problem hiding this comment.
Yes, ideally it would be like that. But for now I think it works fine like this. I think we should refactor this later.
There was a problem hiding this comment.
Sure, lets minimize the changes in this patch.
Lets see if the commit comments appear here