BUG: resolve invalid grep with env neutral script #29551
Conversation
tools/ci/check_c_api_usage.py
Outdated
| + r")(?!\w)") | ||
|
|
||
| NOQA_OK = "noqa: borrowed-ref OK" | ||
| NOQA_MANUAL = "noqa: borrowed-ref - manual fix needed" |
There was a problem hiding this comment.
I suppose it is minor, but some of these module-level variables get used directly in functions without passing them in, which is a bit global-like. I wonder if it might make sense to pass them through. I guess it matters less for non-library code--I might be slightly tempted to just pass them through main and to the function that uses them from there.
There was a problem hiding this comment.
ok I'll move these global variables to the main function.
tools/ci/check_c_api_usage.py
Outdated
| return 2 | ||
|
|
||
| exts = set(args.ext) if args.ext else set(DEFAULT_EXTS) | ||
| excludes = set(args.exclude) if args.exclude else set(DEFAULT_EXCLUDES) |
There was a problem hiding this comment.
these DEFAULT_.. vars are already sets, right?
There was a problem hiding this comment.
yes, I'll adjust these as well. Currently these are global vars but I'll bring them into main.
There was a problem hiding this comment.
yes, I'll adjust these as well. Currently these are global vars but I'll bring them into main.
tools/ci/check_c_api_usage.py
Outdated
| suffix=".txt", | ||
| dir=tmpdir | ||
| ) | ||
| os.close(fd) |
There was a problem hiding this comment.
I suppose we could try to use a context manager to avoid the manual resource handling
tools/ci/check_c_api_usage.py
Outdated
| "--ext", | ||
| action="append", | ||
| default=None, | ||
| help="File extension(s) to include (repeatable). Defaults to .c,.h,.c.src,.cpp", |
There was a problem hiding this comment.
I wonder if it would make sense to just substitute the variable in directly here to avoid hardcoding the defaults in two separate locations, i.e.,
--- a/tools/ci/check_c_api_usage.py
+++ b/tools/ci/check_c_api_usage.py
@@ -148,7 +148,7 @@ def main(argv: list[str] | None = None) -> int:
"--ext",
action="append",
default=None,
- help="File extension(s) to include (repeatable). Defaults to .c,.h,.c.src,.cpp",
+ help=f"File extension(s) to include (repeatable). Defaults to {DEFAULT_EXTS}",There was a problem hiding this comment.
ok making this change.
tools/linter.py
Outdated
| borrowed_res = subprocess.run( | ||
| ["bash", borrowed_ref_script], | ||
| ["python3", borrowed_ref_script], |
There was a problem hiding this comment.
Would it be slightly safer to use sys.executable here? I'm reminded of a recent discussion (scipy/scipy#23334) about preferring that in general with spin.
There was a problem hiding this comment.
ok added this sys.executable in the script
| @@ -0,0 +1,211 @@ | |||
| #!/usr/bin/env python3 | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
Is this just needed in case someone is using an older Python version than we normally support outside of linting?
There was a problem hiding this comment.
Yeah, I think intiially the issue popped up because I didn't think about a certain environment where this script may be used. So I thought maybe this would be helpful.
There was a problem hiding this comment.
I am open to removing this. I don't know if my initial assumption is correct or appropriate for the codebase. But lmk what your preference is here.
|
Hi @tylerjereddy, I made a few changes based on your comments. Please let me know if any other changes are needed before I can merge this PR. |
Attempts to resolve #29538