The outputs provided by the Python version should be exactly the same as the ones from the shell version.
There is small improvement here: Previously only the dependency of flake9 was checked, now all dependencies are checked before running.
I also tried to mostly follow the recommendations here but happy to make more changes if there is still room for improvement.
89+ 'W606' # 'async' and 'await' are reserved keywords starting with Python 3.7
90+)
91+
92+
93+def check_dependencies():
94+ list_out = subprocess.check_output(['pip3', 'list'], stderr=subprocess.DEVNULL).decode('utf-8')
No need to call out to pip here, Python provides introspection for this, but I think even this would work:
0for dep in DEPS:
1 try:
2 __import__(dep)
3 except ImportError:
4 print(f"Skipping Python linting since {dep} is not installed.")
5 exit(0)
pkg_resources. I implemented it using that.
pyzmq is required for linting. I understand why this is though, avoiding that would require a stub library and isn’t in scope here.
11+import os
12+import subprocess
13+import sys
14+
15+DEPS = ['flake8', 'mypy', 'pyzmq']
16+MYPY_CACHE_DIR = f"{os.getenv('BASE_ROOT_DIR', '')}/test/.mypy_cache"
/test/.mypy_cache, which may not be wanted?
getenv with the empty fallback because I wanted to potentially change it to a better one but forgot to think about it. What would you suggest as a better fallback?
84+ 'W601,' # .has_key() is deprecated, use "in"
85+ 'W602,' # deprecated form of raising exception
86+ 'W603,' # "<>" is deprecated, use "!="
87+ 'W604,' # backticks are deprecated, use "repr()"
88+ 'W605,' # invalid escape sequence "x"
89+ 'W606' # 'async' and 'await' are reserved keywords starting with Python 3.7
0 'W606', # 'async' and 'await' are reserved keywords starting with Python 3.7
125+
126+ try:
127+ mypy_out = subprocess.check_output(mypy_args, stderr=subprocess.STDOUT)
128+ except subprocess.CalledProcessError as e:
129+ print(e.output.decode("utf-8"), end="")
130+ exit(1)
0 subprocess.check_call(mypy_args)
1 except subprocess.CalledProcessError:
2 exit(1)
How is this different from the shorter alternative?
See:
check_call but in my testing I didn’t seem to get the errors to my console. But I guess I did something wrong because with your code it seems to work now. Changed.
I was half hoping we could just call the flake8 api directly, and check our files, rather than just moving to wrapping subprocess invocations of flake8 in python instead of bash. Maybe something for the future.
This sounds neat for a follow-up, but personally at least I think as a first step calling out to tools like flake8 is fine. The biggest issue with subprocess is shell ambiguity (so avoid shell=True), and basic system commands that may differ.
I think it’s slightly weird that
pyzmqis required for linting. I understand why this is though, avoiding that would require a stub library and isn’t in scope here.
I am slightly unsure if you would like me to make a change or not, but I think not. Otherwise, let me know :)
I was thinking about this quite a bit when initially putting it on the list but after seeing the errors produced when pyzmq is missing I thought it would be better to make this explicit because otherwise it might lead to people wasting time debugging in the wrong direction.
117+ flake8_out = subprocess.run(flake8_args, stdout=subprocess.PIPE, env=flake8_env)
118+ except subprocess.CalledProcessError as e:
119+ print(e.output.decode("utf-8"), end="")
120+ exit(1)
121+
122+ print(flake8_out.stdout.decode("utf-8"), end="")
how is this different from
0 try:
1 subprocess.check_call(flake8_args, env=flake8_env)
2 except subprocess.CalledProcessError:
3 exit(1)
run allowed to pass in a env argument but it seems it works and is just not explicitly named for check_call. So I changed this.