Merge lp:~aaron-whitehouse/duplicity/08-unadorned-strings into lp:~duplicity-team/duplicity/0.8-series

Proposed by Aaron Whitehouse
Status: Merged
Merged at revision: 1306
Proposed branch: lp:~aaron-whitehouse/duplicity/08-unadorned-strings
Merge into: lp:~duplicity-team/duplicity/0.8-series
Prerequisite: lp:~aaron-whitehouse/duplicity/08-pycodestyle
Diff against target: 340 lines (+253/-39)
2 files modified
testing/find_unadorned_strings.py (+73/-0)
testing/test_code.py (+180/-39)
To merge this branch: bzr merge lp:~aaron-whitehouse/duplicity/08-unadorned-strings
Reviewer Review Type Date Requested Status
duplicity-team Pending
Review via email: mp+347721@code.launchpad.net

Commit message

* Added new script to find unadorned strings (testing/find_unadorned_strings.py python_file) which prints all unadorned strings in a .py file.
* Added a new test to test_code.py that checks across all files for unadorned strings and gives an error if any are found (most files are in an ignore list at this stage, but this will allow us to incrementally remove the exceptions as we adorn the strings in each file).
* Adorn string literals in test_code.py with u/b

Description of the change

As set out in the Python 3 blueprint: https://blueprints.launchpad.net/duplicity/+spec/python3
one of the most time consuming, and least easy to automate, parts of supporting both Python 2 and 3 is string literals. This is because simple strings (e.g. a = "Hello") will be treated as bytes (e.g. encoded ASCII) in Python 2 and Unicode in Python 3. As we are trying to support both Python 2 and Python 3 for at least a transition period, we may end up with odd behaviour wherever we have an unadorned string.

The versions of Python 2 and 3 we are targeting means that we can "adorn" strings with letters to indicate what type of string (u for Unicode, b for Bytes and r for Raw/regexes).

An important preliminary step to Python 2/3 support is therefore for us to add these adornments to each and every string literal in the code base.

To ensure that we can find these and do not accidentally introduce more unadorned strings, this merge request adds a function to our test_code that automatically checks all .py files for unadorned strings and gives an error if any are found.

The actual work to adorn all of these strings will be substantial, so that is not all done in this merge request. Instead, this takes the approach we have for many of our other code style checks, where it currently contains a very long list of excluded files (which are not checked) and we can remove these exceptions as we adorn the strings in each file.

To assist people in finding and correcting all of the unadorned strings in a particular file, the new file testing/find_unadorned_strings.py can be executed directly with a python file as an argument:
./find_unadorned_strings python_file.py
and it will return a nicely-formatted list of all unadorned strings in the file that need to be corrected.

As the codebase is currently Python 2 only, marking strings as Bytes (b" ") essentially preserves current behaviour, but it is highly desirable to convert as many of these as possible to Unicode strings (u" "), as these will be much easier to work with as we transition to Python 3 and it will improve non-ASCII support. This will likely require changes to other parts of the code that interact with the string. The broad recommended approach for text is to decode at the boundaries (e.g. when reading from or writing to files) and use Unicode throughout internally. Many built-ins and libraries natively support Unicode, so in many cases very little needs to change to the code.

Many helper variables/functions have already been created in duplicity so that you can use Unicode wherever possible. For paths, for example, you can use Path.uname instead of Path.name.

To post a comment you must log in.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'testing/find_unadorned_strings.py'
2--- testing/find_unadorned_strings.py 1970-01-01 00:00:00 +0000
3+++ testing/find_unadorned_strings.py 2018-06-08 21:22:09 +0000
4@@ -0,0 +1,73 @@
5+#!/usr/bin/env python3
6+# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
7+#
8+# Copyright 2018 Aaron Whitehouse <aaron@whitehouse.kiwi.nz>
9+#
10+# This file is part of duplicity.
11+#
12+# Duplicity is free software; you can redistribute it and/or modify it
13+# under the terms of the GNU General Public License as published by the
14+# Free Software Foundation; either version 2 of the License, or (at your
15+# option) any later version.
16+#
17+# Duplicity is distributed in the hope that it will be useful, but
18+# WITHOUT ANY WARRANTY; without even the implied warranty of
19+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
20+# General Public License for more details.
21+#
22+# You should have received a copy of the GNU General Public License
23+# along with duplicity; if not, write to the Free Software Foundation,
24+# Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
25+
26+# For predictable results in python2/3 all string literals need to be marked as unicode, bytes or raw
27+# This code finds all unadorned string literals (strings that are not marked with a u, b or r)
28+
29+import sys
30+import tokenize
31+import token
32+
33+# Unfortunately Python2 does not have the useful named tuple result from tokenize.tokenize,
34+# so we have to recreate the effect using namedtuple and tokenize.generate_tokens
35+from collections import namedtuple
36+Python2_token = namedtuple(u'Python2_token', u'type string start end line')
37+
38+
39+def return_unadorned_string_tokens(f):
40+ if sys.version_info[0] < 3:
41+ unnamed_tokens = tokenize.generate_tokens(f.readline)
42+ for t in unnamed_tokens:
43+ named_token = Python2_token(token.tok_name[t[0]], *t[1:])
44+ if named_token.type == u"STRING" and named_token.string[0] in [u'"', u"'"]:
45+ yield named_token
46+
47+ else:
48+ named_tokens = tokenize.tokenize(f.readline)
49+ for t in named_tokens:
50+ if t.type == token.STRING and t.string[0] in [u'"', u"'"]:
51+ yield t
52+
53+
54+def check_file_for_unadorned(python_file):
55+ unadorned_string_list = []
56+ with open(python_file, u'rb') as f:
57+ for s in return_unadorned_string_tokens(f):
58+ unadorned_string_list.append((python_file, s.start, s.end, s.string))
59+ return unadorned_string_list
60+
61+
62+if __name__ == u"__main__":
63+ import argparse
64+
65+ parser = argparse.ArgumentParser(description=u'Find any unadorned string literals in a Python file')
66+ parser.add_argument(u'file', help=u'The file to search')
67+ args = parser.parse_args()
68+
69+ unadorned_string_list = check_file_for_unadorned(args.file)
70+ if len(unadorned_string_list) == 0:
71+ print(u"There are no unadorned strings in", args.file)
72+ else:
73+ print(u"There are unadorned strings in", args.file, u"\n")
74+ for unadorned_string in unadorned_string_list:
75+ print(unadorned_string)
76+ python_file, string_start, string_end, string = unadorned_string
77+ print(string_start, string)
78
79=== modified file 'testing/test_code.py'
80--- testing/test_code.py 2017-12-13 21:03:13 +0000
81+++ testing/test_code.py 2018-06-08 21:22:09 +0000
82@@ -21,16 +21,19 @@
83 import os
84 import subprocess
85 import pytest
86+import fnmatch
87+import os
88
89-if os.getenv('RUN_CODE_TESTS', None) == '1':
90+if os.getenv(u'RUN_CODE_TESTS', None) == u'1':
91 # Make conditional so that we do not have to import in environments that
92 # do not run the tests (e.g. the build servers)
93 import pycodestyle
94
95 from . import _top_dir, DuplicityTestCase # @IgnorePep8
96+from . import find_unadorned_strings
97
98-skipCodeTest = pytest.mark.skipif(not os.getenv('RUN_CODE_TESTS', None) == '1',
99- reason='Must set environment var RUN_CODE_TESTS=1')
100+skipCodeTest = pytest.mark.skipif(not os.getenv(u'RUN_CODE_TESTS', None) == u'1',
101+ reason=u'Must set environment var RUN_CODE_TESTS=1')
102
103
104 class CodeTest(DuplicityTestCase):
105@@ -41,61 +44,199 @@
106 stderr=subprocess.PIPE)
107 output = process.communicate()[0]
108 self.assertTrue(process.returncode in returncodes, output)
109- self.assertEqual("", output, output)
110+ self.assertEqual(u"", output, output)
111
112 @skipCodeTest
113 def test_2to3(self):
114 # As we modernize the source code, we can remove more and more nofixes
115 self.run_checker([
116- "2to3",
117- "--nofix=next",
118- "--nofix=types",
119- "--nofix=unicode",
120+ u"2to3",
121+ u"--nofix=next",
122+ u"--nofix=types",
123+ u"--nofix=unicode",
124 # The following fixes we don't want to remove, since they are false
125 # positives, things we don't care about, or real incompatibilities
126 # but which 2to3 can fix for us better automatically.
127- "--nofix=callable",
128- "--nofix=dict",
129- "--nofix=future",
130- "--nofix=imports",
131- "--nofix=print",
132- "--nofix=raw_input",
133- "--nofix=urllib",
134- "--nofix=xrange",
135+ u"--nofix=callable",
136+ u"--nofix=dict",
137+ u"--nofix=future",
138+ u"--nofix=imports",
139+ u"--nofix=print",
140+ u"--nofix=raw_input",
141+ u"--nofix=urllib",
142+ u"--nofix=xrange",
143 _top_dir])
144
145 @skipCodeTest
146 def test_pylint(self):
147- """Pylint test (requires pylint to be installed to pass)"""
148+ u"""Pylint test (requires pylint to be installed to pass)"""
149 self.run_checker([
150- "pylint",
151- "-E",
152- "--msg-template={msg_id}: {line}: {msg}",
153- "--disable=E0203", # Access to member before its definition line
154- "--disable=E0602", # Undefined variable
155- "--disable=E0611", # No name in module
156- "--disable=E1101", # Has no member
157- "--disable=E1103", # Maybe has no member
158- "--disable=E0712", # Catching an exception which doesn't inherit from BaseException
159- "--ignore=_librsync.so",
160- "--msg-template='{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}'",
161- os.path.join(_top_dir, 'duplicity'),
162- os.path.join(_top_dir, 'bin/duplicity'),
163- os.path.join(_top_dir, 'bin/rdiffdir')],
164+ u"pylint",
165+ u"-E",
166+ u"--msg-template={msg_id}: {line}: {msg}",
167+ u"--disable=E0203", # Access to member before its definition line
168+ u"--disable=E0602", # Undefined variable
169+ u"--disable=E0611", # No name in module
170+ u"--disable=E1101", # Has no member
171+ u"--disable=E1103", # Maybe has no member
172+ u"--disable=E0712", # Catching an exception which doesn't inherit from BaseException
173+ u"--ignore=_librsync.so",
174+ u"--msg-template='{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}'",
175+ os.path.join(_top_dir, u'duplicity'),
176+ os.path.join(_top_dir, u'bin/duplicity'),
177+ os.path.join(_top_dir, u'bin/rdiffdir')],
178 # Allow usage errors, older versions don't have
179 # --msg-template
180 [0, 32])
181
182 @skipCodeTest
183 def test_pep8(self):
184- """Test that we conform to PEP-8 using pycodestyle."""
185+ u"""Test that we conform to PEP-8 using pycodestyle."""
186 # Note that the settings, ignores etc for pycodestyle are set in tox.ini, not here
187- style = pycodestyle.StyleGuide(config_file=os.path.join(_top_dir, 'tox.ini'))
188- result = style.check_files([os.path.join(_top_dir, 'duplicity'),
189- os.path.join(_top_dir, 'bin/duplicity'),
190- os.path.join(_top_dir, 'bin/rdiffdir')])
191+ style = pycodestyle.StyleGuide(config_file=os.path.join(_top_dir, u'tox.ini'))
192+ result = style.check_files([os.path.join(_top_dir, u'duplicity'),
193+ os.path.join(_top_dir, u'bin/duplicity'),
194+ os.path.join(_top_dir, u'bin/rdiffdir')])
195 self.assertEqual(result.total_errors, 0,
196- "Found %s code style errors (and warnings)." % result.total_errors)
197-
198-if __name__ == "__main__":
199+ u"Found %s code style errors (and warnings)." % result.total_errors)
200+
201+ @skipCodeTest
202+ def test_unadorned_string_literals(self):
203+ u"""For predictable results in python2/3 all string literals need to be marked as unicode, bytes or raw"""
204+
205+ ignored_files = [os.path.join(_top_dir, u'.tox', u'*'), # These are not source files we want to check
206+ os.path.join(_top_dir, u'.eggs', u'*'),
207+ # TODO Every file from here down needs to be fixed and the exclusion removed
208+ os.path.join(_top_dir, u'setup.py'),
209+ os.path.join(_top_dir, u'docs', u'conf.py'),
210+ os.path.join(_top_dir, u'duplicity', u'__init__.py'),
211+ os.path.join(_top_dir, u'duplicity', u'asyncscheduler.py'),
212+ os.path.join(_top_dir, u'duplicity', u'backend.py'),
213+ os.path.join(_top_dir, u'duplicity', u'backends', u'__init__.py'),
214+ os.path.join(_top_dir, u'duplicity', u'backends', u'_boto_multi.py'),
215+ os.path.join(_top_dir, u'duplicity', u'backends', u'_boto_single.py'),
216+ os.path.join(_top_dir, u'duplicity', u'backends', u'_cf_cloudfiles.py'),
217+ os.path.join(_top_dir, u'duplicity', u'backends', u'_cf_pyrax.py'),
218+ os.path.join(_top_dir, u'duplicity', u'backends', u'acdclibackend.py'),
219+ os.path.join(_top_dir, u'duplicity', u'backends', u'adbackend.py'),
220+ os.path.join(_top_dir, u'duplicity', u'backends', u'azurebackend.py'),
221+ os.path.join(_top_dir, u'duplicity', u'backends', u'b2backend.py'),
222+ os.path.join(_top_dir, u'duplicity', u'backends', u'botobackend.py'),
223+ os.path.join(_top_dir, u'duplicity', u'backends', u'cfbackend.py'),
224+ os.path.join(_top_dir, u'duplicity', u'backends', u'dpbxbackend.py'),
225+ os.path.join(_top_dir, u'duplicity', u'backends', u'gdocsbackend.py'),
226+ os.path.join(_top_dir, u'duplicity', u'backends', u'giobackend.py'),
227+ os.path.join(_top_dir, u'duplicity', u'backends', u'hsibackend.py'),
228+ os.path.join(_top_dir, u'duplicity', u'backends', u'hubicbackend.py'),
229+ os.path.join(_top_dir, u'duplicity', u'backends', u'imapbackend.py'),
230+ os.path.join(_top_dir, u'duplicity', u'backends', u'jottacloudbackend.py'),
231+ os.path.join(_top_dir, u'duplicity', u'backends', u'lftpbackend.py'),
232+ os.path.join(_top_dir, u'duplicity', u'backends', u'localbackend.py'),
233+ os.path.join(_top_dir, u'duplicity', u'backends', u'mediafirebackend.py'),
234+ os.path.join(_top_dir, u'duplicity', u'backends', u'megabackend.py'),
235+ os.path.join(_top_dir, u'duplicity', u'backends', u'multibackend.py'),
236+ os.path.join(_top_dir, u'duplicity', u'backends', u'ncftpbackend.py'),
237+ os.path.join(_top_dir, u'duplicity', u'backends', u'onedrivebackend.py'),
238+ os.path.join(_top_dir, u'duplicity', u'backends', u'par2backend.py'),
239+ os.path.join(_top_dir, u'duplicity', u'backends', u'pcabackend.py'),
240+ os.path.join(_top_dir, u'duplicity', u'backends', u'pydrivebackend.py'),
241+ os.path.join(_top_dir, u'duplicity', u'backends', u'pyrax_identity', u'__init__.py'),
242+ os.path.join(_top_dir, u'duplicity', u'backends', u'pyrax_identity', u'hubic.py'),
243+ os.path.join(_top_dir, u'duplicity', u'backends', u'rsyncbackend.py'),
244+ os.path.join(_top_dir, u'duplicity', u'backends', u'ssh_paramiko_backend.py'),
245+ os.path.join(_top_dir, u'duplicity', u'backends', u'ssh_pexpect_backend.py'),
246+ os.path.join(_top_dir, u'duplicity', u'backends', u'swiftbackend.py'),
247+ os.path.join(_top_dir, u'duplicity', u'backends', u'sxbackend.py'),
248+ os.path.join(_top_dir, u'duplicity', u'backends', u'tahoebackend.py'),
249+ os.path.join(_top_dir, u'duplicity', u'backends', u'webdavbackend.py'),
250+ os.path.join(_top_dir, u'duplicity', u'cached_ops.py'),
251+ os.path.join(_top_dir, u'duplicity', u'collections.py'),
252+ os.path.join(_top_dir, u'duplicity', u'commandline.py'),
253+ os.path.join(_top_dir, u'duplicity', u'compilec.py'),
254+ os.path.join(_top_dir, u'duplicity', u'diffdir.py'),
255+ os.path.join(_top_dir, u'duplicity', u'dup_temp.py'),
256+ os.path.join(_top_dir, u'duplicity', u'dup_threading.py'),
257+ os.path.join(_top_dir, u'duplicity', u'dup_time.py'),
258+ os.path.join(_top_dir, u'duplicity', u'errors.py'),
259+ os.path.join(_top_dir, u'duplicity', u'file_naming.py'),
260+ os.path.join(_top_dir, u'duplicity', u'filechunkio.py'),
261+ os.path.join(_top_dir, u'duplicity', u'globals.py'),
262+ os.path.join(_top_dir, u'duplicity', u'globmatch.py'),
263+ os.path.join(_top_dir, u'duplicity', u'gpg.py'),
264+ os.path.join(_top_dir, u'duplicity', u'gpginterface.py'),
265+ os.path.join(_top_dir, u'duplicity', u'lazy.py'),
266+ os.path.join(_top_dir, u'duplicity', u'librsync.py'),
267+ os.path.join(_top_dir, u'duplicity', u'log.py'),
268+ os.path.join(_top_dir, u'duplicity', u'manifest.py'),
269+ os.path.join(_top_dir, u'duplicity', u'patchdir.py'),
270+ os.path.join(_top_dir, u'duplicity', u'path.py'),
271+ os.path.join(_top_dir, u'duplicity', u'progress.py'),
272+ os.path.join(_top_dir, u'duplicity', u'robust.py'),
273+ os.path.join(_top_dir, u'duplicity', u'selection.py'),
274+ os.path.join(_top_dir, u'duplicity', u'statistics.py'),
275+ os.path.join(_top_dir, u'duplicity', u'tarfile.py'),
276+ os.path.join(_top_dir, u'duplicity', u'tempdir.py'),
277+ os.path.join(_top_dir, u'duplicity', u'util.py'),
278+ os.path.join(_top_dir, u'testing', u'__init__.py'),
279+ os.path.join(_top_dir, u'testing', u'find_unadorned_strings.py'),
280+ os.path.join(_top_dir, u'testing', u'functional', u'__init__.py'),
281+ os.path.join(_top_dir, u'testing', u'functional', u'test_badupload.py'),
282+ os.path.join(_top_dir, u'testing', u'functional', u'test_cleanup.py'),
283+ os.path.join(_top_dir, u'testing', u'functional', u'test_final.py'),
284+ os.path.join(_top_dir, u'testing', u'functional', u'test_log.py'),
285+ os.path.join(_top_dir, u'testing', u'functional', u'test_rdiffdir.py'),
286+ os.path.join(_top_dir, u'testing', u'functional', u'test_replicate.py'),
287+ os.path.join(_top_dir, u'testing', u'functional', u'test_restart.py'),
288+ os.path.join(_top_dir, u'testing', u'functional', u'test_selection.py'),
289+ os.path.join(_top_dir, u'testing', u'functional', u'test_verify.py'),
290+ os.path.join(_top_dir, u'testing', u'manual', u'__init__.py'),
291+ os.path.join(_top_dir, u'testing', u'overrides', u'__init__.py'),
292+ os.path.join(_top_dir, u'testing', u'overrides', u'gettext.py'),
293+ os.path.join(_top_dir, u'testing', u'test_unadorned.py'),
294+ os.path.join(_top_dir, u'testing', u'unit', u'__init__.py'),
295+ os.path.join(_top_dir, u'testing', u'unit', u'test_backend_instance.py'),
296+ os.path.join(_top_dir, u'testing', u'unit', u'test_backend.py'),
297+ os.path.join(_top_dir, u'testing', u'unit', u'test_collections.py'),
298+ os.path.join(_top_dir, u'testing', u'unit', u'test_diffdir.py'),
299+ os.path.join(_top_dir, u'testing', u'unit', u'test_dup_temp.py'),
300+ os.path.join(_top_dir, u'testing', u'unit', u'test_dup_time.py'),
301+ os.path.join(_top_dir, u'testing', u'unit', u'test_file_naming.py'),
302+ os.path.join(_top_dir, u'testing', u'unit', u'test_globmatch.py'),
303+ os.path.join(_top_dir, u'testing', u'unit', u'test_gpg.py'),
304+ os.path.join(_top_dir, u'testing', u'unit', u'test_gpginterface.py'),
305+ os.path.join(_top_dir, u'testing', u'unit', u'test_lazy.py'),
306+ os.path.join(_top_dir, u'testing', u'unit', u'test_manifest.py'),
307+ os.path.join(_top_dir, u'testing', u'unit', u'test_patchdir.py'),
308+ os.path.join(_top_dir, u'testing', u'unit', u'test_path.py'),
309+ os.path.join(_top_dir, u'testing', u'unit', u'test_selection.py'),
310+ os.path.join(_top_dir, u'testing', u'unit', u'test_statistics.py'),
311+ os.path.join(_top_dir, u'testing', u'unit', u'test_tarfile.py'),
312+ os.path.join(_top_dir, u'testing', u'unit', u'test_tempdir.py')]
313+
314+
315+ # Find all the .py files in the duplicity tree
316+ # We cannot use glob.glob recursive until we drop support for Python < 3.5
317+ matches = []
318+
319+ def multi_filter(names, patterns):
320+ u"""Generator function which yields the names that match one or more of the patterns."""
321+ for name in names:
322+ if any(fnmatch.fnmatch(name, pattern) for pattern in patterns):
323+ yield name
324+
325+ for root, dirnames, filenames in os.walk(_top_dir):
326+ for filename in fnmatch.filter(filenames, u'*.py'):
327+ matches.append(os.path.join(root, filename))
328+
329+ excluded = multi_filter(matches, ignored_files) if ignored_files else []
330+ matches = list(set(matches) - set(excluded))
331+
332+ for python_source_file in matches:
333+ # Check each of the relevant python sources for unadorned string literals
334+ unadorned_string_list = find_unadorned_strings.check_file_for_unadorned(python_source_file)
335+ self.assertEqual([], unadorned_string_list,
336+ u"Found %s unadorned strings: \n %s" % (len(unadorned_string_list), unadorned_string_list))
337+
338+
339+if __name__ == u"__main__":
340 unittest.main()

Subscribers

People subscribed via source and target branches