Skip to content
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

DOC SignalManager docstrings. See GH-713. #1291

Merged
merged 2 commits into from Jun 10, 2015
Merged

Conversation

@kmike
Copy link
Member

@kmike kmike commented Jun 8, 2015

This change is not 100% backwards compatible because of *args changes. Usage of *args is not documented, so we're not breaking the public interface. Users of *args will likely find their code broken if we change (#8) our signals implementation anyways.

Generated docs look almost the same as existing; the only difference is that order in which methods are documented is changed.

I got tired looking up what arguments SignalManager methods use - our signals docs is a maze, and source code is not helpful because of *a, **kw. Thus this PR :)

See also: #713

This change is not 100% backwards compatible because of *args changes.
Their usage was not documented, so we're not breaking public interface.
@curita
Copy link
Member

@curita curita commented Jun 9, 2015

I got bitten by this too, +1 to this change.

By looking a little looks like *args isn't commonly used (I didn't find a single call of the SignalManager methods without named arguments in our repositories), so it should be a rather safe backward incompatibility.

def send_catch_log(self, *a, **kw):
kw.setdefault('sender', self.sender)
return signal.send_catch_log(*a, **kw)
.. _deferreds: http://twistedmatrix.com/documents/current/core/howto/defer.html

This comment has been minimized.

@curita

curita Jun 9, 2015
Member

Shouldn't be _deferred (singular) here? Both mentions of this link are singular, but I think the first line of the docstring of send_catch_log_deferred should be changed too, it was "... but supports returning deferreds_ from ..." before.

Probably the links _deferred and _deferredsat the bottom of docs/topics/api.rst should be deleted.

This comment has been minimized.

@kmike

kmike Jun 9, 2015
Author Member

Good catches, fixed. I don't like polluting a docstring with 2 identical links under slightly different names, so the second one is removed.

@dangra
Copy link
Member

@dangra dangra commented Jun 10, 2015

LGTM but I don't think we should backport to 1.0.

@kmike
Copy link
Member Author

@kmike kmike commented Jun 10, 2015

Just noticed one more backwards-incompatible change here: previously user was able to omit 'signal' argument in all methods, now it raises an exception. I don't see an use case for omitting 'signal' argument.

Found it by calling signals.disconnect_all(), expecting it to disconnect all signal handlers. But this doesn't disconnect all handlers, so a new exception may help to found such bugs.

@kmike
Copy link
Member Author

@kmike kmike commented Jun 10, 2015

from scrapy.xlib.pydispatch import dispatcher
from scrapy.utils.signal import disconnect_all

sender = object()
signal = object()

def handler():
    print("hello")

dispatcher.connect(handler, signal=signal, sender=sender)
dispatcher.send(signal, sender=sender)  # prints 

disconnect_all(sender=sender)
dispatcher.send(signal, sender=sender)  # prints

disconnect_all(signal=signal, sender=sender)
dispatcher.send(signal, sender=sender)  # doesn't print
@kmike
Copy link
Member Author

@kmike kmike commented Jun 10, 2015

I'm fine with not backporting it to 1.0.

@dangra
Copy link
Member

@dangra dangra commented Jun 10, 2015

previous implementation desconnects all handlers, I see it defaults to Any object declared in pydispatcher source code.

@kmike
Copy link
Member Author

@kmike kmike commented Jun 10, 2015

The example above shows that signal_manager.disconnect_all() doesn't disconnect all handlers. The code emulates SignalManager - SignalManager passes sender to all its functions.

@dangra
Copy link
Member

@dangra dangra commented Jun 10, 2015

@kmike you're right.

@dangra
Copy link
Member

@dangra dangra commented Jun 10, 2015

Should we merge now or are you going to add an exception/warning for the no-signal corner case?

@kmike
Copy link
Member Author

@kmike kmike commented Jun 10, 2015

After this PR an exception is raised if user omits signal because it is no longer optional in function signature.

I don't know, maybe there are users of default signal value, so maybe a warning is better. On the other hand, we never documented that a signal can be omitted; I prefer to keep the exception.

dangra added a commit that referenced this pull request Jun 10, 2015
DOC SignalManager docstrings. See GH-713.
@dangra dangra merged commit 5bd0395 into master Jun 10, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@dangra dangra deleted the signalmanager-docstrings branch Jun 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants