| Message ID | 2ae621feecea77223421234eaef0de7032c841d1.1528016895.git.yann.morin.1998@free.fr |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/5] support/docker: run apt-get update and apt-get install in two RUNs | expand |
Hello, The endless patch series... each iteration I suggest new patches for it :-) + Thomas P There is nothing really wrong with the changes this patch do to the code. But see below 2 nits that could lead us (or not) to choose one way or another and standardize all Python scripts in the tree. On Sun, Jun 03, 2018 at 06:08 AM, Yann E. MORIN wrote: > Fix three issues with code style in our test infra: > - 'print' is now a function, > - exceptions need to be caught-assigned with the 'as' keyword, > - old-style "%s"%() formatting is deprecated. This patch also do: - add indices in format string We currently do that only for check-uniq-files. Everywhere else we use '{}' instead of '{0}' when possible, as this is not really needed for Python 3 when we use each arguments once in the same order they are passed to format(). BUT... looking at "62fa5e17cb support/scripts/check-uniq-files: add indices in format string" maybe it would be good to adopt a simplistic policy of always using indices in format string, as it seems to be compatible to Python >= 2.6 and then we don't need to think about whether the script will be used during the build (and therefore need to support old distros) or not when reviewing. Thoughts? [snip] > - print "Downloading to {}".format(tmpfile) > + print("Downloading to {0}".format(tmpfile)) ^ Example of index mentioned above. 2 more occurrences in the patch. [snip] > - print "Missing download directory, please use -d/--download" > - print "" > + print("Missing download directory, please use -d/--download") > + print("") I was about to suggest to change to 'print()' as it is already used in scanpypi. But I didn't tested yet any form on python2.6. 5 more occurrences in the patch. Thoughts? Regards, Ricardo
Hello, On Mon, 04 Jun 2018 23:17:12 -0300, Ricardo Martincoski wrote: > This patch also do: > - add indices in format string > > We currently do that only for check-uniq-files. > Everywhere else we use '{}' instead of '{0}' when possible, as this is not > really needed for Python 3 when we use each arguments once in the same order > they are passed to format(). > > BUT... looking at "62fa5e17cb support/scripts/check-uniq-files: add indices in > format string" maybe it would be good to adopt a simplistic policy of always > using indices in format string, as it seems to be compatible to Python >= 2.6 > and then we don't need to think about whether the script will be used during > the build (and therefore need to support old distros) or not when reviewing. > > Thoughts? I am not sure it is useful (and realistic) to have all our Python scripts compatible with Python 2.6. This puts a pretty hard constraint which might be annoying down the road. So Python scripts involved in the build itself should definitely be compatible with Python 2.6, but all the utility Python scripts around that are not essential for the build, I don't think we should enforce this constraint. Best regards, Thomas
Hello, On Sun, 3 Jun 2018 11:08:21 +0200, Yann E. MORIN wrote: > Fix three issues with code style in our test infra: > - 'print' is now a function, > - exceptions need to be caught-assigned with the 'as' keyword, > - old-style "%s"%() formatting is deprecated. > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com> > > --- > Changes v1 -> v2: > - this was previously caught because of a spurious switch to > python3-flake8, as noticed by Ricardo. We're now back to using > python-flake8, so the rationale has changed. Still, we do want > to fix those because it makes sense anyway. > --- > support/testing/infra/__init__.py | 6 +++--- > support/testing/infra/basetest.py | 4 ++-- > support/testing/run-tests | 26 +++++++++++++------------- > 3 files changed, 18 insertions(+), 18 deletions(-) I've dropped the changes that added indices to format strings (since they would only be needed for Python 2.6 compatibility, and I'm not sure that it's a worthwhile goal to be able to run the test suite on Python 2.6 machines). And then I've applied to master. Thanks! Thomas
diff --git a/support/testing/infra/__init__.py b/support/testing/infra/__init__.py index b03e891771..b0f28de450 100644 --- a/support/testing/infra/__init__.py +++ b/support/testing/infra/__init__.py @@ -34,17 +34,17 @@ def download(dldir, filename): os.makedirs(dldir) tmpfile = tempfile.mktemp(dir=dldir) - print "Downloading to {}".format(tmpfile) + print("Downloading to {0}".format(tmpfile)) try: url_fh = urlopen(os.path.join(ARTIFACTS_URL, filename)) with open(tmpfile, "w+") as tmpfile_fh: tmpfile_fh.write(url_fh.read()) - except (HTTPError, URLError), err: + except (HTTPError, URLError) as err: os.unlink(tmpfile) raise err - print "Renaming from %s to %s" % (tmpfile, finalpath) + print("Renaming from {0} to {1}".format(tmpfile, finalpath)) os.rename(tmpfile, finalpath) return finalpath diff --git a/support/testing/infra/basetest.py b/support/testing/infra/basetest.py index f3f13ad97f..82756afefd 100644 --- a/support/testing/infra/basetest.py +++ b/support/testing/infra/basetest.py @@ -46,8 +46,8 @@ class BRTest(unittest.TestCase): self.config += "\nBR2_JLEVEL={}\n".format(self.jlevel) def show_msg(self, msg): - print "{} {:40s} {}".format(datetime.datetime.now().strftime("%H:%M:%S"), - self.testname, msg) + print("{0} {1:40s} {2}".format(datetime.datetime.now().strftime("%H:%M:%S"), + self.testname, msg)) def setUp(self): self.show_msg("Starting") diff --git a/support/testing/run-tests b/support/testing/run-tests index 270e78cff7..76dd15e9f0 100755 --- a/support/testing/run-tests +++ b/support/testing/run-tests @@ -41,7 +41,7 @@ def main(): BRTest.logtofile = False if args.list: - print "List of tests" + print("List of tests") nose2.discover(argv=[script_path, "-s", test_dir, "-v", @@ -52,16 +52,16 @@ def main(): if args.download is None: args.download = os.getenv("BR2_DL_DIR") if args.download is None: - print "Missing download directory, please use -d/--download" - print "" + print("Missing download directory, please use -d/--download") + print("") parser.print_help() return 1 BRTest.downloaddir = os.path.abspath(args.download) if args.output is None: - print "Missing output directory, please use -o/--output" - print "" + print("Missing output directory, please use -o/--output") + print("") parser.print_help() return 1 @@ -71,8 +71,8 @@ def main(): BRTest.outputdir = os.path.abspath(args.output) if args.all is False and len(args.testname) == 0: - print "No test selected" - print "" + print("No test selected") + print("") parser.print_help() return 1 @@ -80,8 +80,8 @@ def main(): if args.testcases != 1: if args.testcases < 1: - print "Invalid number of testcases to run simultaneously" - print "" + print("Invalid number of testcases to run simultaneously") + print("") parser.print_help() return 1 # same default BR2_JLEVEL as package/Makefile.in @@ -93,16 +93,16 @@ def main(): if args.jlevel: if args.jlevel < 0: - print "Invalid BR2_JLEVEL to use for each testcase" - print "" + print("Invalid BR2_JLEVEL to use for each testcase") + print("") parser.print_help() return 1 # the user can override the auto calculated value BRTest.jlevel = args.jlevel if args.timeout_multiplier < 1: - print "Invalid multiplier for timeout values" - print "" + print("Invalid multiplier for timeout values") + print("") parser.print_help() return 1 BRTest.timeout_multiplier = args.timeout_multiplier
Fix three issues with code style in our test infra: - 'print' is now a function, - exceptions need to be caught-assigned with the 'as' keyword, - old-style "%s"%() formatting is deprecated. Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com> --- Changes v1 -> v2: - this was previously caught because of a spurious switch to python3-flake8, as noticed by Ricardo. We're now back to using python-flake8, so the rationale has changed. Still, we do want to fix those because it makes sense anyway. --- support/testing/infra/__init__.py | 6 +++--- support/testing/infra/basetest.py | 4 ++-- support/testing/run-tests | 26 +++++++++++++------------- 3 files changed, 18 insertions(+), 18 deletions(-)