Merge lp://staging/~barry/autopilot/lp1488175 into lp://staging/autopilot

Proposed by Barry Warsaw
Status: Needs review
Proposed branch: lp://staging/~barry/autopilot/lp1488175
Merge into: lp://staging/autopilot
Diff against target: 912 lines (+242/-119) (has conflicts)
23 files modified
autopilot/introspection/_xpathselect.py (+4/-7)
autopilot/process/_bamf.py (+4/-2)
autopilot/run.py (+8/-8)
autopilot/testcase.py (+2/-2)
autopilot/tests/functional/test_ap_apps.py (+2/-1)
autopilot/tests/functional/test_autopilot_functional.py (+4/-2)
autopilot/tests/functional/test_dbus_query.py (+8/-10)
autopilot/tests/functional/test_input_stack.py (+9/-1)
autopilot/tests/unit/test_application_launcher.py (+11/-10)
autopilot/tests/unit/test_input.py (+4/-1)
autopilot/tests/unit/test_introspection_dbus.py (+4/-1)
autopilot/tests/unit/test_introspection_xpathselect.py (+33/-19)
autopilot/tests/unit/test_pick_backend.py (+20/-17)
autopilot/tests/unit/test_platform.py (+2/-1)
autopilot/tests/unit/test_process.py (+2/-2)
autopilot/tests/unit/test_stagnate_state.py (+6/-3)
autopilot/tests/unit/test_test_loader.py (+4/-3)
autopilot/tests/unit/test_testcase.py (+5/-4)
autopilot/tests/unit/test_types.py (+17/-8)
autopilot/vis/dbus_search.py (+10/-6)
debian/changelog (+73/-5)
docs/otto.py (+1/-4)
setup.py (+9/-2)
Text conflict in autopilot/tests/functional/test_input_stack.py
Text conflict in setup.py
To merge this branch: bzr merge lp://staging/~barry/autopilot/lp1488175
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Needs Fixing
Autopilot Hackers Pending
Review via email: mp+268967@code.staging.launchpad.net

Description of the change

My take on fixing the flake8 failures.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:519
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~barry/autopilot/lp1488175/+merge/268967/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/autopilot-ci/1133/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/63/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/62/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/63/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/autopilot-ci/1133/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Max Brustkern (nuclearbob) wrote :

I have a couple of questions, but other than the merge conflicts, most of it looks great.

Revision history for this message
Barry Warsaw (barry) wrote :
Download full text (3.2 KiB)

On Aug 27, 2015, at 08:22 PM, Max Brustkern wrote:

>> @@ -39,8 +40,7 @@
>> """Must return a backend when called with a single backend."""
>> class Backend(object):
>> pass
>> - _create_backend = lambda: Backend()
>> - backend = _pick_backend(dict(foo=_create_backend), '')
>> + backend = _pick_backend(dict(foo=Backend), '')
>
>I'm not quite a clever in python as everyone else here, so I just want to
>make sure that Backend is wanted here and not Backend() This seems to apply
>to most of the tests around this, so I imagine they'd be breaking if it were
>incorrect, I just like to be sure it's intentional.

It is!

lambdas are defined here:

https://docs.python.org/3/reference/expressions.html#lambda

The example given provides a great way to think about them.

    lambda args: expression

is equivalent to

    def <lambda>(args):
        return expression

where the function is anonymous, i.e. it performs no name binding in the
current namespace. When you assign the `lambda: Backend()` to
_create_backend, it's the assignment that does the name binding to the
function object, so its exactly equivalent to:

    def _create_backend():
        return Backend()

Note too that when _pick_backend() is called, the code is passing in a
dictionary that maps the `foo` key to the *function object* created by the
lambda; specifically, the calling of the lambda-created function object is
deferred. It's exactly equivalent to just setting `foo` to the Backend
function object without calling it.

So in this case, the lambda is really just creating an unnecessary extra level
of call.

That's not quite the case where some of the lambdas take an argument, but you
can see how the translation to explicit function definitions should generally
go.

One thing to keep in mind about my translations. To be accurate, the
lambda-to-def conversion should return the value of the last expression, but I
don't do this in all cases because the tests appear to only care that an
exception is raised. Thus, the return values are mostly ignored. But for
correctness, you could return the values out of the locally defined
functions.

BTW, this equivalence is the reason why lambda are generally frowned upon.
They're almost never needed, and the savings in lines of code are usually not
worth the cost in readability or comprehension. It was probably the fans of
functional programming that tipped the balance against just removing them from
Python 3. ;)

>> === modified file 'setup.py'
>> --- setup.py 2015-08-19 00:25:00 +0000
>> +++ setup.py 2015-08-24 19:50:50 +0000
>> @@ -20,9 +20,16 @@
>> from setuptools import find_packages, setup, Extension
>>
>> import sys
>> +
>> +from setuptools import find_packages, setup, Extension
>
>It surprises me that these are needed and haven't been included
>previously. Was there something causing static analysis to not pick this up
>before?

Oh, that's weird. I don't remember adding that. Maybe it's a merge snafu?

P.S. you can play with this to see the equivalence in action:

def runme(func):
    print(func())

def hey():
    return 'hey'

yo = lambda: hey()

def sup():
    return hey()

run...

Read more...

Revision history for this message
Max Brustkern (nuclearbob) wrote :

> On Aug 27, 2015, at 08:22 PM, Max Brustkern wrote:
>
> >> @@ -39,8 +40,7 @@
> >> """Must return a backend when called with a single backend."""
> >> class Backend(object):
> >> pass
> >> - _create_backend = lambda: Backend()
> >> - backend = _pick_backend(dict(foo=_create_backend), '')
> >> + backend = _pick_backend(dict(foo=Backend), '')
> >
> >I'm not quite a clever in python as everyone else here, so I just want to
> >make sure that Backend is wanted here and not Backend() This seems to apply
> >to most of the tests around this, so I imagine they'd be breaking if it were
> >incorrect, I just like to be sure it's intentional.
>
> It is!
>
> lambdas are defined here:
>
> https://docs.python.org/3/reference/expressions.html#lambda
>
> The example given provides a great way to think about them.
>
> lambda args: expression
>
> is equivalent to
>
> def <lambda>(args):
> return expression
>
> where the function is anonymous, i.e. it performs no name binding in the
> current namespace. When you assign the `lambda: Backend()` to
> _create_backend, it's the assignment that does the name binding to the
> function object, so its exactly equivalent to:
>
> def _create_backend():
> return Backend()
>
> Note too that when _pick_backend() is called, the code is passing in a
> dictionary that maps the `foo` key to the *function object* created by the
> lambda; specifically, the calling of the lambda-created function object is
> deferred. It's exactly equivalent to just setting `foo` to the Backend
> function object without calling it.

Yeah, that's what was confusing me. I saw:
_create_backend = lambda: Backend()
and assumed that would return Backend() (the return value of that, i.e. an instance of the class) and not Backend (the class itself) The rest of the lambda syntax made sense to me, I just tripped over some parentheses here, since we're setting foo=Backend and not foo=Backend() like I would have expected from the lambda. Exciting stuff!

Unmerged revisions

519. By Barry Warsaw

A few more flake8 fixes.

518. By Barry Warsaw

Mass fix for LP: #1488175

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches