Merge lp:~elopio/dialer-app/fix1250270-pep8 into lp:dialer-app
- fix1250270-pep8
- Merge into trunk
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 | ||||
Related bugs: |
|
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.
Description of the change
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_
> 70 + "dialer-
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_
>
> This style is also making things worse. Could you split it like
>
> CALL_DURATION = config.
> 'outgoing_
>
> 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.
Gustavo Pichorim Boiko (boiko) wrote : | # |
Looks good!
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:86
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:87
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:88
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Martin Pitt (pitti) wrote : | # |
Failed due to package not being installable, which looked like a temporary problem in -proposed. Rebuilding.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:88
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Martin Pitt (pitti) wrote : | # |
The evolution-
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:88
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Loïc Minier (lool) wrote : | # |
FTR, I've carefully reviewed the diff and ran pep8 myself and this looked good.
Preview Diff
1 | === modified file 'tests/autopilot/dialer_app/emulators.py' | |||
2 | --- tests/autopilot/dialer_app/emulators.py 2013-10-01 16:57:37 +0000 | |||
3 | +++ tests/autopilot/dialer_app/emulators.py 2013-11-22 19:46:15 +0000 | |||
4 | @@ -46,7 +46,8 @@ | |||
5 | 46 | "*": "buttonAsterisk", | 46 | "*": "buttonAsterisk", |
6 | 47 | "#": "buttonHash", | 47 | "#": "buttonHash", |
7 | 48 | } | 48 | } |
9 | 49 | return self.select_single("KeypadButton", objectName=buttonsDict[number]) | 49 | return self.select_single("KeypadButton", |
10 | 50 | objectName=buttonsDict[number]) | ||
11 | 50 | 51 | ||
12 | 51 | def get_erase_button(self): | 52 | def get_erase_button(self): |
13 | 52 | return self.select_single("CustomButton", objectName="eraseButton") | 53 | return self.select_single("CustomButton", objectName="eraseButton") |
14 | 53 | 54 | ||
15 | === modified file 'tests/autopilot/dialer_app/tests/__init__.py' | |||
16 | --- tests/autopilot/dialer_app/tests/__init__.py 2013-10-16 14:34:02 +0000 | |||
17 | +++ tests/autopilot/dialer_app/tests/__init__.py 2013-11-22 19:46:15 +0000 | |||
18 | @@ -1,5 +1,5 @@ | |||
19 | 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 -*- |
21 | 2 | # Copyright 2012 Canonical | 2 | # Copyright 2012, 2013 Canonical |
22 | 3 | # | 3 | # |
23 | 4 | # This file is part of dialer-app. | 4 | # This file is part of dialer-app. |
24 | 5 | # | 5 | # |
25 | @@ -25,6 +25,7 @@ | |||
26 | 25 | 25 | ||
27 | 26 | logger = logging.getLogger(__name__) | 26 | logger = logging.getLogger(__name__) |
28 | 27 | 27 | ||
29 | 28 | |||
30 | 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, |
31 | 29 | # otherwise we consider this a test failure (missing dependencies) | 30 | # otherwise we consider this a test failure (missing dependencies) |
32 | 30 | def tp_has_ofono(): | 31 | def tp_has_ofono(): |
33 | @@ -82,7 +83,8 @@ | |||
34 | 82 | else: | 83 | else: |
35 | 83 | self.app = self.launch_test_application( | 84 | self.app = self.launch_test_application( |
36 | 84 | "dialer-app", | 85 | "dialer-app", |
38 | 85 | "--desktop_file_hint=/usr/share/applications/dialer-app.desktop", | 86 | "--desktop_file_hint=" |
39 | 87 | "/usr/share/applications/dialer-app.desktop", | ||
40 | 86 | app_type='qt', | 88 | app_type='qt', |
41 | 87 | emulator_base=toolkit_emulators.UbuntuUIToolkitEmulatorBase) | 89 | emulator_base=toolkit_emulators.UbuntuUIToolkitEmulatorBase) |
42 | 88 | 90 | ||
43 | 89 | 91 | ||
44 | === modified file 'tests/autopilot/dialer_app/tests/test_calls.py' | |||
45 | --- tests/autopilot/dialer_app/tests/test_calls.py 2013-11-06 18:20:50 +0000 | |||
46 | +++ tests/autopilot/dialer_app/tests/test_calls.py 2013-11-22 19:46:15 +0000 | |||
47 | @@ -34,7 +34,7 @@ | |||
48 | 34 | @skipUnless(have_phonesim, | 34 | @skipUnless(have_phonesim, |
49 | 35 | "this test needs to run under with-ofono-phonesim") | 35 | "this test needs to run under with-ofono-phonesim") |
50 | 36 | @skipIf(os.uname()[2].endswith("maguro"), | 36 | @skipIf(os.uname()[2].endswith("maguro"), |
52 | 37 | "tests cause Unity crashes on maguro") | 37 | "tests cause Unity crashes on maguro") |
53 | 38 | class TestCalls(DialerAppTestCase): | 38 | class TestCalls(DialerAppTestCase): |
54 | 39 | """Tests for simulated phone calls.""" | 39 | """Tests for simulated phone calls.""" |
55 | 40 | 40 | ||
56 | @@ -132,10 +132,11 @@ | |||
57 | 132 | self.wait_for_incoming_call() | 132 | self.wait_for_incoming_call() |
58 | 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 :-) |
59 | 134 | subprocess.check_call( | 134 | subprocess.check_call( |
64 | 135 | ["dbus-send", "--session", "--print-reply", | 135 | [ |
65 | 136 | "--dest=com.canonical.Approver", "/com/canonical/Approver", | 136 | "dbus-send", "--session", "--print-reply", |
66 | 137 | "com.canonical.TelephonyServiceApprover.AcceptCall"], | 137 | "--dest=com.canonical.Approver", "/com/canonical/Approver", |
67 | 138 | stdout=subprocess.PIPE) | 138 | "com.canonical.TelephonyServiceApprover.AcceptCall" |
68 | 139 | ], stdout=subprocess.PIPE) | ||
69 | 139 | 140 | ||
70 | 140 | # call back is from that number | 141 | # call back is from that number |
71 | 141 | self.wait_live_call_page("1234567") | 142 | self.wait_live_call_page("1234567") |
72 | @@ -147,7 +148,8 @@ | |||
73 | 147 | try: | 148 | try: |
74 | 148 | self.hangup() | 149 | self.hangup() |
75 | 149 | except MismatchError as e: | 150 | except MismatchError as e: |
77 | 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 ' |
78 | 152 | '(https://launchpad.net/bugs/1240400): %s' % e) | ||
79 | 151 | 153 | ||
80 | 152 | # | 154 | # |
81 | 153 | # Helper methods | 155 | # Helper methods |
82 | @@ -166,7 +168,8 @@ | |||
83 | 166 | 168 | ||
84 | 167 | Sets self.hangup_button. | 169 | Sets self.hangup_button. |
85 | 168 | """ | 170 | """ |
87 | 169 | self.hangup_button = self.app.wait_select_single(objectName="hangupButton") | 171 | self.hangup_button = self.app.wait_select_single( |
88 | 172 | objectName="hangupButton") | ||
89 | 170 | self.assertThat(self.hangup_button.visible, Eventually(Equals(True))) | 173 | self.assertThat(self.hangup_button.visible, Eventually(Equals(True))) |
90 | 171 | self.assertThat(self.call_button.visible, Equals(False)) | 174 | self.assertThat(self.call_button.visible, Equals(False)) |
91 | 172 | 175 | ||
92 | @@ -192,7 +195,6 @@ | |||
93 | 192 | # on desktop, notify-osd generates a persistent popup, clean this up | 195 | # on desktop, notify-osd generates a persistent popup, clean this up |
94 | 193 | subprocess.call(['pkill', '-f', 'notify-osd']) | 196 | subprocess.call(['pkill', '-f', 'notify-osd']) |
95 | 194 | 197 | ||
96 | 195 | |||
97 | 196 | def hangup(self): | 198 | def hangup(self): |
98 | 197 | self.pointing_device.click_object(self.hangup_button) | 199 | self.pointing_device.click_object(self.hangup_button) |
99 | 198 | 200 |
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/ " app.desktop" ,
70 + "dialer-
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( variables' , 'outgoing_ call_duration' )
17 + 'connected_
This style is also making things worse. Could you split it like
CALL_DURATION = config. getint( 'connected_ variables' ,
'outgoing_ call_duration' )
instead?
Thanks!