[PATCH] D67814: [LNT] Python 3 support: adapt to dict method returning views

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
Comment at: examples/run_to_csv.py:10
 
-titles = data['tests'].itervalues().next().keys()
+titles = iter(data['tests'].values()).next().keys()
 titles.insert(0, titles.pop(titles.index("name")))
----------------
hubert.reinterpretcast wrote:
> I believe this use of `next()` is one of the ones that need to be replaced in the style of D67812. This file seems to be unmaintained and untested. None of the `*.json` files in the `tests/` directory seems to match the format this script expects. I would suggest deleting this file.
I prefer to apply the change for now and remove it or fix it in a later patch if that's ok. Strange the .next logic didn't detect this case. I've double checked the whole codebase for a call to .next() and didn't find any in python files.


================
Comment at: lnt/lnttool/admin.py:50
             config = yaml.load(open(filename))
-            for key, value in config.items():
+            for key, value in list(config.items()):
                 self._set(key, value)
----------------
hubert.reinterpretcast wrote:
> I believe the change on this line is unnecessary. I'm not marking further instances, but we generally shouldn't form a list just to iterate.
Is there no multithreading going on in LNT?


================
Comment at: lnt/server/ui/views.py:958
 
         data.sort(key=lambda sample: convert_revision(sample[0], cache=revision_cache))
 
----------------
hubert.reinterpretcast wrote:
> Is this use of `sort` something that would be considered within the scope of D67809?
I think the 2to3 fix_idioms only fires if sort is called without parameter. Presumably if the call to sort takes some parameters that might make the folding both lines into sorted too long and thus be difficult to read. I think in this specific case it would indeed be harder to read so I'm enclined to leave it like this.


================
Comment at: lnt/server/ui/views.py:1497
     # Get the list of available test suites.
-    testsuites = request.get_db().testsuite.values()
+    testsuites = list(request.get_db().testsuite.values())
 
----------------
testsuites is only used for iterating below so I removed the list here.


================
Comment at: lnt/testing/__init__.py:260
         # Convert keys/values that are not json encodable to strings.
         for key, value in list(info.items()):
             key = str(key)
----------------
I'm removing the list here which I added when committing https://reviews.llvm.org/D65751


================
Comment at: lnt/tests/nt.py:1483
                          if x.is_rerunable()]
     rerunable_benches.sort(key=lambda x: x.name)
     # Now lets do the reruns.
----------------
hubert.reinterpretcast wrote:
> Another call to `sort`. Maybe use something like the following instead?
> 
> ```
> sorted(filter(lambda bench: bench.is_rerunable(),
>               collated_results.values()),
>        key=lambda bench: bench.name)
> ```
As mentioned, the 2to3 sort change seems to skip this sort of folding intentionally, presumably to avoid a multiline expression. I'm therefore sticking to having filter and sort as separate steps. I did however changed to use the filter builtin as it makes the intent clearer IMHO.


================
Comment at: lnt/util/multidict.py:19-26
     def items(self):
-        return self.data.items()
+        return list(self.data.items())
 
     def values(self):
-        return self.data.values()
+        return list(self.data.values())
 
     def keys(self):
----------------
All uses of these methods already assume this returns views and have been converted to use list if necessary. Ultimately we should remove that file and rely on the pypi package IMHO.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67814/new/

https://reviews.llvm.org/D67814