dialer-app

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

fix1250270-pep8
Merge into trunk

Proposed by Leo Arias on 2013-11-12

Status:

Merged

Approved by:

Oliver Grawert on 2013-11-28

Approved revision:

Merged at revision:

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:

Bug #1250270: Pep8 errors

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Loïc Minier			Approve on 2013-11-28
PS Jenkins bot	continuous-integration		Approve on 2013-11-28
Martin Pitt			Approve on 2013-11-27
Gustavo Pichorim Boiko (community)		2013-11-12	Approve on 2013-11-12
Review via email: mp+194775@code.launchpad.net

Commit message

Fix pep8 errors.

Revision history for this message

Martin Pitt (pitti) wrote on 2013-11-12:

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 on 2013-11-12:

> 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 on 2013-11-12:

Looks good!

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-17:

review: Needs Fixing (continuous-integration)

lp:~elopio/dialer-app/fix1250270-pep8 updated on 2013-11-17

85. By Leo Arias on 2013-11-17: Merged with trunk.
86. By Leo Arias on 2013-11-17: Fixed invalid syntax.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-17:

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

review: Needs Fixing (continuous-integration)

lp:~elopio/dialer-app/fix1250270-pep8 updated on 2013-11-17

87. By Leo Arias on 2013-11-17: Fixed typo.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-17:

FAILED: Continuous integration, rev:87
http://jenkins.qa.ubuntu.com/job/dialer-app-ci/109/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-amd64-ci/14
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-armhf-ci/14
        deb: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-armhf-ci/14/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dialer-app-trusty-i386-ci/14
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/711
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/697
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/647
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/711
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/711/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/697
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/697/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/3327
    SUCCESS: http://s-jenkins:8080/job/touch-flash-device/1416

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

review: Needs Fixing (continuous-integration)

lp:~elopio/dialer-app/fix1250270-pep8 updated on 2013-11-22

88. By Leo Arias on 2013-11-22: Merged with trunk.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-22:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Martin Pitt (pitti) wrote on 2013-11-27:

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 on 2013-11-27:

LGTM now.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-27:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Martin Pitt (pitti) wrote on 2013-11-28:

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 on 2013-11-28:

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

review: Approve (continuous-integration)

Revision history for this message

Loïc Minier (lool) wrote on 2013-11-28:

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

Subscribers

People subscribed via source and target branches

to all changes:

Ishan Joshi

Leo Arias

Ubuntu Phablet Team

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	"*": "buttonAsterisk",
6	"#": "buttonHash",
7	}
8	- return self.select_single("KeypadButton", objectName=buttonsDict[number])
9	+ return self.select_single("KeypadButton",
10	+ objectName=buttonsDict[number])
11
12	def get_erase_button(self):
13	return self.select_single("CustomButton", objectName="eraseButton")
14
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	# -- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 --
20	-# Copyright 2012 Canonical
21	+# Copyright 2012, 2013 Canonical
22	#
23	# This file is part of dialer-app.
24	#
25	@@ -25,6 +25,7 @@
26
27	logger = logging.getLogger(__name__)
28
29	+
30	# ensure we have an ofono account; we assume that we have these tools,
31	# otherwise we consider this a test failure (missing dependencies)
32	def tp_has_ofono():
33	@@ -82,7 +83,8 @@
34	else:
35	self.app = self.launch_test_application(
36	"dialer-app",
37	- "--desktop_file_hint=/usr/share/applications/dialer-app.desktop",
38	+ "--desktop_file_hint="
39	+ "/usr/share/applications/dialer-app.desktop",
40	app_type='qt',
41	emulator_base=toolkit_emulators.UbuntuUIToolkitEmulatorBase)
42
43
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	@skipUnless(have_phonesim,
49	"this test needs to run under with-ofono-phonesim")
50	@skipIf(os.uname()[2].endswith("maguro"),
51	- "tests cause Unity crashes on maguro")
52	+ "tests cause Unity crashes on maguro")
53	class TestCalls(DialerAppTestCase):
54	"""Tests for simulated phone calls."""
55
56	@@ -132,10 +132,11 @@
57	self.wait_for_incoming_call()
58	time.sleep(1) # let's hear the ringing sound for a second :-)
59	subprocess.check_call(
60	- ["dbus-send", "--session", "--print-reply",
61	- "--dest=com.canonical.Approver", "/com/canonical/Approver",
62	- "com.canonical.TelephonyServiceApprover.AcceptCall"],
63	- stdout=subprocess.PIPE)
64	+ [
65	+ "dbus-send", "--session", "--print-reply",
66	+ "--dest=com.canonical.Approver", "/com/canonical/Approver",
67	+ "com.canonical.TelephonyServiceApprover.AcceptCall"
68	+ ], stdout=subprocess.PIPE)
69
70	# call back is from that number
71	self.wait_live_call_page("1234567")
72	@@ -147,7 +148,8 @@
73	try:
74	self.hangup()
75	except MismatchError as e:
76	- print('Expected failure due to known Mir crash (https://launchpad.net/bugs/1240400): %s' % e)
77	+ print('Expected failure due to known Mir crash '
78	+ '(https://launchpad.net/bugs/1240400): %s' % e)
79
80	#
81	# Helper methods
82	@@ -166,7 +168,8 @@
83
84	Sets self.hangup_button.
85	"""
86	- self.hangup_button = self.app.wait_select_single(objectName="hangupButton")
87	+ self.hangup_button = self.app.wait_select_single(
88	+ objectName="hangupButton")
89	self.assertThat(self.hangup_button.visible, Eventually(Equals(True)))
90	self.assertThat(self.call_button.visible, Equals(False))
91
92	@@ -192,7 +195,6 @@
93	# on desktop, notify-osd generates a persistent popup, clean this up
94	subprocess.call(['pkill', '-f', 'notify-osd'])
95
96	-
97	def hangup(self):
98	self.pointing_device.click_object(self.hangup_button)
99

dialer-app

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

Commit message

Description of the change

Preview Diff

Subscribers