Code Review: fix pylint/pyflakes errors

[bkabrda]

Hi, so I was asked to do a detailed code review for MTF, so here's the first issue - code style and statically checked warnings errors.

Since the outputs that I got from running pylint/pyflakes are quite sizable, I'll just comment on some general points. You can install these tools yourselves (pip install --user pylint pyflakes) and run them on the code (pylint moduleframework/, pyflakes moduleframework/) to see what's wrong. So here's some general comments from my code observations and pylint/pyflakes output:

• [ ] When inheriting from a parent class, you should call it's __init__ method in the child's __init__. Occurs in multiple places, e.g. moduleframework.mtf_generator:TestGenerator.__init__ should call super(TestGenerator, self).__init__().
• [ ] pylint complains about method names being camelcase, not snakecase. The general recommendation in Python is using snakecase, but camelcase is also fine as long as you stay consistent throughout the codebase. For example, in moduleframework.setup, there are some methods whose names begin in capital letters, some functions in moduleframework.common and moduleframework.dockerlinter use snakecase etc. I recommend choosing one naming scheme and applying it everywhere. The same applies to variable naming.
• [ ] pylint reports some bad indentation, e.g. in moduleframework.module_framework, line 1400-1404. While Python 2 can generally guess what you meant with e.g. 3 spaces, Python 3 will report this as syntax error, so I'd recommend fixing these.
• [ ] Generally, strings representing regular expressions should be raw strings. A good explanation of why is at the very beginning section of [2].
• [ ] You should use absolute imports, not relative imports. This is mostly important because of Python 3 compatibility. E.g. instead of from compose_info import .., use from moduleframework.compose_info import ... (this appears throughout the codebase).
• [ ] You should not use wildcard imports (from foo import *). These pose a danger of polluting your namespace and overriding methods with generic names that you import from elsewhere. You should always import exactly what you need (this appears throughout the codebase).
• [ ] In Python, keyword argument assignment in function definitions/calls should not have empty spaces around =, so self.__addModuleDependency(url=latesturl, name = dep, ... is wrong, self.__addModuleDependency(url=latesturl, name=dep, .. is correct. This occurs in multiple places. Also, exactly one space is required after comma that separates arguments.
• [ ] moduleframework.module_framework, line 316 has wrong mode for open (rw doesn't exist in Python). Judging from the code, you probably wanted to use just r. Also, the file isn't closed anywhere.
• [ ] pylint reports some "catching too general exception Exception" in quite a lot of places. While this is occasionally ok to do, I'd recommend narrowing it down to a subclass if possible.
• [ ] pylint is reporting some unnecessary pass statements, these can be safely removed.
• [ ] pylint also complains about lines being long. Generally this is a matter of taste, but lately the consensus in Python community has been to use 100 chars per line at maximum.
• [ ] pylint complains about lots of "invalid variable names", mostly for variables that are called x, a, b, etc. foo is not a great name either. While this is sometimes ok, I'd recommend considering using more descriptive names.
• [ ] pylint reports problems with import order. Generally, the imports should be as follows: 1) imports from stdlib, 2) imports from pip-installed dependencies, 3) imports from this module.
• [ ] pylint complains about some redefined top-level variables, but I think these are actually mostly fine to keep.
• [ ] pylint complains about some object attributes being defined outside of __init__, mostly in moduleframework.pdc_data. While this may sometimes make sense, I'd highly recommend initializing them to None or similar values inside __init__.
• [ ] pylint complains about lots of unused arguments. While this may actually be part of making sure API stays stable in the future, it may also be a sign of ignoring important input, so I'd advise investigating these.
• [ ] pylint reports lots of "Wrong hanging indentation" errors. Even though this is not a huge issue, fixing this may improve code readability. Generally speaking, the hanging indentation should follow rules outlined at [1].
• [ ] pylint reports couple of unused variables throughout the codebase, these can most likely be safely removed.
• [ ] pylint reports that some modules and classes miss docstrings, but I don't consider this to be a huge issue. I would however recommend providing docstrings for all methods (some docstrings are missing).

I'd highly recommend registering at some CI and adding runs of pylint/pyflakes to make check in order to keep code quality high.

[1] https://www.python.org/dev/peps/pep-0008/#indentation [2] https://docs.python.org/2/library/re.html#module-re

[alexxa]

Hi, @bkabrda . Many thanks for the comprehensive code review. Can you please share who asked you to do it? The thing is that I came to the project a while ago, and did the same, and I'm aware of all the issues you wrote here as well as mtf as a single command, no need for makefiles, etc and etc, and working on them, just never reported all of those as an issue here, but in words to the other project members. Yeah, my bad, I should. And also, are you just reporting or also going to fix those? I just want to avoid further work duplication

[bkabrda]

@alexxa sure, @phracek asked me to do it :) Since I'm pretty occupied otherwise, I won't be working towards fixing these.

[TomasTomecek]

I actually initiated the work of doing complete review of mtf. The reason behind it is very simple: if we are suppose to use mtf to test modules and containers, I wanna make sure that mtf has high-quality code with polished workflows and is doing the right things.

Edit: looking in trello, I second Slavek: code review was initiated by Petr, not me.

[alexxa]

@bkabrda @TomasTomecek thanks. It seems we have a communication issue with @phracek =( @TomasTomecek +1 on remark about enhancing MTF code quality.

[phracek]

@bkabrda @TomasTomecek @alexxa This is a bit longer issue. I would split it into more issues. What do you think?

[TomasTomecek]

I would probably turn original Slavek's post into checklist and just mark which items are already solved,

• [ ] something
• [ ] like
• [x] this