Merge lp:~elopio/dialer-app/fix1250270-pep8 into lp:dialer-app

Proposed by Leo Arias
Status: Merged
Approved by: Oliver Grawert
Approved revision: 88
Merged at revision: 80
Proposed branch: lp:~elopio/dialer-app/fix1250270-pep8
Merge into: lp:dialer-app
Diff against target: 98 lines (+16/-11)
3 files modified
tests/autopilot/dialer_app/emulators.py (+2/-1)
tests/autopilot/dialer_app/tests/__init__.py (+4/-2)
tests/autopilot/dialer_app/tests/test_calls.py (+10/-8)
To merge this branch: bzr merge lp:~elopio/dialer-app/fix1250270-pep8
Reviewer Review Type Date Requested Status
Loïc Minier Approve
PS Jenkins bot continuous-integration Approve
Martin Pitt Approve
Gustavo Pichorim Boiko (community) Approve
Review via email: mp+194775@code.launchpad.net

Commit message

Fix pep8 errors.

To post a comment you must log in.
Revision history for this message
Martin Pitt (pitti) wrote :

Ah, the annoying 79 char limit from the 1970... :-( This makes code harder to read, but *shrug*, we have the policy to obey to it, so let's do that.

69 + "--desktop_file_hint=/usr/share/applications/"
70 + "dialer-app.desktop",

Please don't split paths, it makes them much harder to copy&paste or open the file in vim ("gf"). Please split it after the "=" instead.

16 + CALL_DURATION = config.getint(
17 + 'connected_variables', 'outgoing_call_duration')

This style is also making things worse. Could you split it like

CALL_DURATION = config.getint('connected_variables',
                              'outgoing_call_duration')

instead?

Thanks!

review: Needs Fixing
Revision history for this message
Leo Arias (elopio) wrote :

> Ah, the annoying 79 char limit from the 1970... :-( This makes code harder to
> read, but *shrug*, we have the policy to obey to it, so let's do that.
>
> 69 + "--desktop_file_hint=/usr/share/applications/"
> 70 + "dialer-app.desktop",

You are right, I fixed it.

> Please don't split paths, it makes them much harder to copy&paste or open the
> file in vim ("gf"). Please split it after the "=" instead.
>
> 16 + CALL_DURATION = config.getint(
> 17 + 'connected_variables', 'outgoing_call_duration')
>
> This style is also making things worse. Could you split it like
>
> CALL_DURATION = config.getint('connected_variables',
> 'outgoing_call_duration')
>
> instead?

Pep8 lets you choose between these two styles, which is bad because then people start to argue about which is best and it's completely subjective :)
I prefer the one I used, but I also prefer to make a reviewer happy, so I changed it.

> Thanks!

Thanks for your review.

Revision history for this message
Gustavo Pichorim Boiko (boiko) wrote :

Looks good!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/dialer-app-autolanding/65/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-amd64-autolanding/7
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-armhf-autolanding/7
        deb: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-armhf-autolanding/7/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-i386-autolanding/7
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/672
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/658/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/613
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/672
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/672/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/658
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/658/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-maguro/3077/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/3293/console
    SUCCESS: http://s-jenkins:8080/job/touch-flash-device/1374
    SUCCESS: http://s-jenkins:8080/job/touch-flash-device/1373

review: Needs Fixing (continuous-integration)
85. By Leo Arias

Merged with trunk.

86. By Leo Arias

Fixed invalid syntax.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:86
http://jenkins.qa.ubuntu.com/job/dialer-app-ci/108/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-amd64-ci/13
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-armhf-ci/13
        deb: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-armhf-ci/13/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-i386-ci/13
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/684
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/670/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/622
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/684
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/684/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/670
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/670/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-maguro/3086
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/3302/console
    SUCCESS: http://s-jenkins:8080/job/touch-flash-device/1393
    SUCCESS: http://s-jenkins:8080/job/touch-flash-device/1390

Click here to trigger a rebuild:
http://s-jenkins:8080/job/dialer-app-ci/108/rebuild

review: Needs Fixing (continuous-integration)
87. By Leo Arias

Fixed typo.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
88. By Leo Arias

Merged with trunk.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Martin Pitt (pitti) wrote :

Failed due to package not being installable, which looked like a temporary problem in -proposed. Rebuilding.

Revision history for this message
Martin Pitt (pitti) wrote :

LGTM now.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Martin Pitt (pitti) wrote :

The evolution-data-server transition which broke installability of folks and Qt in -proposed finally seems done, re-running.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Loïc Minier (lool) wrote :

FTR, I've carefully reviewed the diff and ran pep8 myself and this looked good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'tests/autopilot/dialer_app/emulators.py'
--- tests/autopilot/dialer_app/emulators.py 2013-10-01 16:57:37 +0000
+++ tests/autopilot/dialer_app/emulators.py 2013-11-22 19:46:15 +0000
@@ -46,7 +46,8 @@
46 "*": "buttonAsterisk",46 "*": "buttonAsterisk",
47 "#": "buttonHash",47 "#": "buttonHash",
48 }48 }
49 return self.select_single("KeypadButton", objectName=buttonsDict[number])49 return self.select_single("KeypadButton",
50 objectName=buttonsDict[number])
5051
51 def get_erase_button(self):52 def get_erase_button(self):
52 return self.select_single("CustomButton", objectName="eraseButton")53 return self.select_single("CustomButton", objectName="eraseButton")
5354
=== modified file 'tests/autopilot/dialer_app/tests/__init__.py'
--- tests/autopilot/dialer_app/tests/__init__.py 2013-10-16 14:34:02 +0000
+++ tests/autopilot/dialer_app/tests/__init__.py 2013-11-22 19:46:15 +0000
@@ -1,5 +1,5 @@
1# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-1# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
2# Copyright 2012 Canonical2# Copyright 2012, 2013 Canonical
3#3#
4# This file is part of dialer-app.4# This file is part of dialer-app.
5#5#
@@ -25,6 +25,7 @@
2525
26logger = logging.getLogger(__name__)26logger = logging.getLogger(__name__)
2727
28
28# ensure we have an ofono account; we assume that we have these tools,29# ensure we have an ofono account; we assume that we have these tools,
29# otherwise we consider this a test failure (missing dependencies)30# otherwise we consider this a test failure (missing dependencies)
30def tp_has_ofono():31def tp_has_ofono():
@@ -82,7 +83,8 @@
82 else:83 else:
83 self.app = self.launch_test_application(84 self.app = self.launch_test_application(
84 "dialer-app",85 "dialer-app",
85 "--desktop_file_hint=/usr/share/applications/dialer-app.desktop",86 "--desktop_file_hint="
87 "/usr/share/applications/dialer-app.desktop",
86 app_type='qt',88 app_type='qt',
87 emulator_base=toolkit_emulators.UbuntuUIToolkitEmulatorBase)89 emulator_base=toolkit_emulators.UbuntuUIToolkitEmulatorBase)
8890
8991
=== modified file 'tests/autopilot/dialer_app/tests/test_calls.py'
--- tests/autopilot/dialer_app/tests/test_calls.py 2013-11-06 18:20:50 +0000
+++ tests/autopilot/dialer_app/tests/test_calls.py 2013-11-22 19:46:15 +0000
@@ -34,7 +34,7 @@
34@skipUnless(have_phonesim,34@skipUnless(have_phonesim,
35 "this test needs to run under with-ofono-phonesim")35 "this test needs to run under with-ofono-phonesim")
36@skipIf(os.uname()[2].endswith("maguro"),36@skipIf(os.uname()[2].endswith("maguro"),
37 "tests cause Unity crashes on maguro")37 "tests cause Unity crashes on maguro")
38class TestCalls(DialerAppTestCase):38class TestCalls(DialerAppTestCase):
39 """Tests for simulated phone calls."""39 """Tests for simulated phone calls."""
4040
@@ -132,10 +132,11 @@
132 self.wait_for_incoming_call()132 self.wait_for_incoming_call()
133 time.sleep(1) # let's hear the ringing sound for a second :-)133 time.sleep(1) # let's hear the ringing sound for a second :-)
134 subprocess.check_call(134 subprocess.check_call(
135 ["dbus-send", "--session", "--print-reply",135 [
136 "--dest=com.canonical.Approver", "/com/canonical/Approver",136 "dbus-send", "--session", "--print-reply",
137 "com.canonical.TelephonyServiceApprover.AcceptCall"],137 "--dest=com.canonical.Approver", "/com/canonical/Approver",
138 stdout=subprocess.PIPE)138 "com.canonical.TelephonyServiceApprover.AcceptCall"
139 ], stdout=subprocess.PIPE)
139140
140 # call back is from that number141 # call back is from that number
141 self.wait_live_call_page("1234567")142 self.wait_live_call_page("1234567")
@@ -147,7 +148,8 @@
147 try:148 try:
148 self.hangup()149 self.hangup()
149 except MismatchError as e:150 except MismatchError as e:
150 print('Expected failure due to known Mir crash (https://launchpad.net/bugs/1240400): %s' % e)151 print('Expected failure due to known Mir crash '
152 '(https://launchpad.net/bugs/1240400): %s' % e)
151153
152 #154 #
153 # Helper methods155 # Helper methods
@@ -166,7 +168,8 @@
166168
167 Sets self.hangup_button.169 Sets self.hangup_button.
168 """170 """
169 self.hangup_button = self.app.wait_select_single(objectName="hangupButton")171 self.hangup_button = self.app.wait_select_single(
172 objectName="hangupButton")
170 self.assertThat(self.hangup_button.visible, Eventually(Equals(True)))173 self.assertThat(self.hangup_button.visible, Eventually(Equals(True)))
171 self.assertThat(self.call_button.visible, Equals(False))174 self.assertThat(self.call_button.visible, Equals(False))
172175
@@ -192,7 +195,6 @@
192 # on desktop, notify-osd generates a persistent popup, clean this up195 # on desktop, notify-osd generates a persistent popup, clean this up
193 subprocess.call(['pkill', '-f', 'notify-osd'])196 subprocess.call(['pkill', '-f', 'notify-osd'])
194197
195
196 def hangup(self):198 def hangup(self):
197 self.pointing_device.click_object(self.hangup_button)199 self.pointing_device.click_object(self.hangup_button)
198200

Subscribers

People subscribed via source and target branches