lint: convert submodule linter test to Python #24803
Conversation
9d6d07e to
1b324cf
Compare
test/lint/lint-submodule.py
Outdated
| try: | ||
| submodules_list = subprocess.check_output(submodule_check_args, stderr = subprocess.STDOUT) | ||
| except subprocess.CalledProcessError: | ||
| exit(1) |
There was a problem hiding this comment.
I think python already returns an exit code of 1 when it raises an exception
There was a problem hiding this comment.
I do think it's neater not to have a Python exception in the output, especially if the subprocess already prints its own self-evident errors.
There was a problem hiding this comment.
This should never happen, and when it happens, the process won't print it's own errors, so the exception seems better?
There was a problem hiding this comment.
I'm pretty sure that git prints its own error when it fails. But ok, no strong opinion here.
There was a problem hiding this comment.
check_output will eat the output. With stderr = subprocess.STDOUT it will also eat the error.
There was a problem hiding this comment.
I hadn't noticed the stderr = subprocess.STDOUT. Pretty sure we don't need it. The original script doesn't redirect the stderr either.
There was a problem hiding this comment.
Thank you @laanwj , @KevinMusgrave and @MarcoFalke for your review and points which led me to dig further.
I second @MarcoFalke .
From my understanding, 2 cases are:
- Without
stderr = subprocess.STDOUT, the error is printed onto the console and is not piped to Python. - With
stderr = subprocess.STDOUT, if we want to access the error message, we can access it usinge.outputin the exception block (whereeis CalledProcessError object). Without explicit printing, error is not printed onto console.
I'll update the PR to print errors to keep the behaviour consistent with older version. Additionally, I don't see any cases where this command will return exit code = 1. Should I remove try exception block ?
Kindly let me know your views too.
Edit: I removed try exception block.
|
Concept/approach ACK |
1b324cf to
3289688
Compare
|
Before change: After change: |
|
Tested ACK 3289688 |
test/lint/lint-submodule.py
Outdated
| return subprocess.check_output(command).decode('utf-8').splitlines() | ||
|
|
||
| def main(): | ||
| submodules_list = get_submodules_list() |
There was a problem hiding this comment.
| submodules_list = get_submodules_list() | |
| submodules_list = subprocess.check_output( |
no need for a function, you can call the stdlib directly
test/lint/lint-submodule.py
Outdated
| submodules_list = get_submodules_list() | ||
| if submodules_list: | ||
| print("These submodules were found, delete them:") | ||
| for submodule in submodules_list: |
There was a problem hiding this comment.
what is the point of first splitting by newline and then joining the array by newline again when printing? Just leave it as string?
There was a problem hiding this comment.
I agree, that was unnecessary. Updated the PR. Thanks for pointing it out !
3289688 to
4a9e36d
Compare
|
Tested ACK 4a9e36d |
4a9e36d lint: convert submodule linter test to Python (Eunoia) Pull request description: Refs bitcoin#24783 ACKs for top commit: laanwj: Tested ACK 4a9e36d Tree-SHA512: ca25b59acf75cebc79588a6c51dc5c313c8d0bd1d492127815d7b81b36aaffd02815a515d97b355582002f71efc33d46435d0b28fce24497bb99799d9ba57228
Refs #24783