> I ran checkers and found several of the following (no need to mark them all > inline): > > tests/storagetest_runner/__init__.py|101 col 16 warning| [unused-variable] > Unused variable '_err' [python/pylint] > > => if really unused a _ would be even better. Yeah, that makes sense. I just got in the habit of writing _err from block meta, but I should probably clean up some of the variable names there too. For the most part the err part of the util.subp return is never used. > tests/storagetest_runner/__init__.py|208 col 18 warning| [logging-format- > interpolation] Use % formatting in logging functions and pass the % parameters > as arguments > > => since log would format for you ... > > I know pylint it is noisy and sometimes even disagrees with other checkers, > but most of these are also only a search and replace away so probably worth to > fix. Yeah, I can use the old style format specifiers there, I'll switch that over. > I'm not so sure about > tests/storagetests/__init__.py|186 col 16 warning| [unidiomatic-typecheck] > Using type() instead of isinstance() for a typecheck. [python/pylint] Right yeah, I'll switch that, isinstance reads nicer. > tests/storagetests/__init__.py|219 col 24 warning| [redefined-builtin] > Redefining built-in 'type' [python/pylint] Oh, yeah, that was for error type in __exit__, I'll rename that to 'etype' > I already complained about short names before, this is another example (there > are more like mp, fp, e, ...) > tests/storagetests/__init__.py|224 col 13 warning| [invalid-name] Invalid > variable name "d" [python/pylint] Yeah, I think some of these are just trying to keep the line from going over 80, but some were probably in the list comprehensions, where the scope of the var is so limited it should be okay. I'll switch everything outside of comprehensions over to long names real quick, thanks for bringing that up. > > A totally different one is: > tests/storagetests/verifiers.py|15 col 13 error| [no-member] Instance of > 'BasicTests' has no 'assertIsNotNone' member [python/pylint] > That never is an issue so far, as all inheriting classes of e.g. BasicTests > also inherit from class BaseStorageTest(unittest.TestCase). But to make sure > the methods are in scope would it hurt letting the verifiers also derive from > unittest.TestCase? I guess the verifiers could inherit from TestCase, but I think it may be cleaner for them not to, partly just because that would require '__test__ = False' in all of them to prevent nosetests3 from trying to run them as individual tests. Also, because the BaseStorageTest class overrides the TestCase.run() method, if the verifier classes were to inherit from TestCase and were listed as parents of the individual test classes after the BaseStorageTest, I think that the call to super() in BaseStorageTest.run() would mess up, but I'm not 100% sure there. I should probably do something like define an __init__ for the verifier classes that checks something like 'if not isinstance(self, TestCase)' and throws an error to make the verifiers unusable outside of the test classes. > One that is probably worth for sure to avoid later issues: > tests/storagetests/__init__.py|82 col 31 warning| [redefined-outer-name] > Redefining name 'reporter' from outer scope (line 2) [python/pylint] Oh yeah, that makes sense, I'll rename that to something else. I don't think it should cause any issues because the inner name is in a context manager and should no longer exist as soon as the context leaves, but it definitely could be confusing if that was rewritten at some point. And actually, looking at ReportEventStack, it is okay to just discard the return of __enter__ and just use the instance itself there.