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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast marked 2 inline comments as done.
hubert.reinterpretcast 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")))
----------------
thopre wrote:
> 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.
Okay.


================
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)
----------------
thopre wrote:
> 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?
`config` is local to the function, so I don't think it is being concurrently modified here.


================
Comment at: lnt/server/ui/views.py:958
 
         data.sort(key=lambda sample: convert_revision(sample[0], cache=revision_cache))
 
----------------
thopre wrote:
> 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.
Okay.


================
Comment at: lnt/tests/nt.py:1488
     logger.info(summary.format(len(rerunable_benches),
-                               len(collated_results.values())))
+                               len(list(collated_results.values()))))
 
----------------
hubert.reinterpretcast wrote:
> `len(collated_results)`
I'm not seeing the change. `len` applied to the dictionary directly works.


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

https://reviews.llvm.org/D67814