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

Don't recommend using addCallback as a function decorator #2523

Merged
merged 2 commits into from Dec 13, 2016
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 23 additions & 0 deletions master/docs/developer/style.rst
Expand Up @@ -96,6 +96,7 @@ code for a particular function or method within the same indented block, using
nested functions::

def getRevInfo(revname):
# for example only! See above a better implementation with inlineCallbacks
results = {}
d = defer.succeed(None)
def rev_parse(_): # note use of '_' to quietly indicate an ignored parameter
Expand All @@ -122,6 +123,15 @@ Deferred. As a shortcut, ``d.addCallback`` works as a decorator::
def rev_parse(_): # note use of '_' to quietly indicate an ignored parameter
return utils.getProcessOutput(git, [ 'rev-parse', revname ])

.. note::

``d.addCallback`` is not really a decorator as it does not return a modified function.
As a result in previous code, ``rev_parse`` value is actually the Deferred.
In general the :class:`inlineCallbacks` method is preferred inside new code as it keeps the code easier to read.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ shouldn't this be something like

twisted:func:`inlineCallbacks` 

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise how will it know where to link to when rendering?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not this is just used for styling. We don't have back reference helpers to twisted docs

As a general rule of thumb, when you need more than 2 callbacks in the same method, its time to switch it to :class:`inlineCallbacks`.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo "it's"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Same here :class:... > ...:func:...

This would be for example the case for previous :py:func:`getRevInfo`.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ And as i understand it, this would attempt to link to the Python docs....should probably just be getRevInfo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, this is just a styling hint that this bloc is a function, if there is no corresponding

.. :py:function:`getRevinfo`.

See this `discussion <https://github.com/buildbot/buildbot/pull/2523>`_ with twisted experts for more information.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Twisted is a proper noun (not an adjective:wink:).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahah! right. You made my day. You guys are really twisted!


Be careful with local variables. For example, if ``parse_rev_parse``, above,
merely assigned ``rev = res.strip()``, then that variable would be local to
``parse_rev_parse`` and not available in ``set_results``. Mutable variables
Expand Down Expand Up @@ -176,6 +186,19 @@ losing any readability.

Note that code using ``deferredGenerator`` is no longer acceptable in Buildbot.

The previous :py:func:`getRevInfo` example implementation should rather be written as:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Same here.


.. code-block::
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of typos but otherwise looks good to me @tardyp

Actually I notice there's a sphinx lint warning reported for this code-block.


@defer.inlineCallbacks
def getRevInfo(revname):
results = {}
res = yield utils.getProcessOutput(git, [ 'rev-parse', revname ])
results['rev'] = res.strip()
res = yield utils.getProcessOutput(git, [ 'log', '-1', '--format=%s%n%b', results['rev'] ])
results['comments'] = res.strip()
defer.returnValue(results)

Locking
.......

Expand Down