Fix license scripts #207
Fix license scripts #207
Conversation
scripts/license/add.py
Outdated
| "git rev-parse --show-toplevel".split()).strip().decode("utf-8")) | ||
| PROJECT_ROOT = subprocess.check_output( | ||
| "git rev-parse --show-toplevel".split()).strip().decode("utf-8") | ||
| target_dirs = [os.path.join(PROJECT_ROOT, d) for d in os.listdir(PROJECT_ROOT) |
There was a problem hiding this comment.
target_dirs is used only in main(). Please consider moving target_dirs into main() to minimize the scope.
scripts/license/add.py
Outdated
| PROJECT_ROOT = subprocess.check_output( | ||
| "git rev-parse --show-toplevel".split()).strip().decode("utf-8") | ||
| target_dirs = [os.path.join(PROJECT_ROOT, d) for d in os.listdir(PROJECT_ROOT) | ||
| if d != "vendor" and os.path.isdir(os.path.join(PROJECT_ROOT, d))] |
There was a problem hiding this comment.
It may be a matter of taste, but using pathlib would make the code easier to read and write. Also, it would maintain consistency with other parts that are using pathlib.
scripts/license/add.py
Outdated
| for d in target_dirs: | ||
| add(Path(d).glob("**/*_k8s.go"), license_header("//", modification=True), verbose) | ||
| add([p for p in Path(d).glob("**/*.go") | ||
| if p.name[-7:] != "_k8s.go"], license_header("//"), verbose) |
There was a problem hiding this comment.
For avoiding a magic number ..
| if p.name[-7:] != "_k8s.go"], license_header("//"), verbose) | |
| if not p.name.endswith("_k8s.go")], license_header("//"), verbose) |
(I wrote the original code 😉 )
| ok &= check(Path(PROJECT_ROOT).glob("*.go"), license_header("//"), verbose) | ||
| ok &= check(Path(PROJECT_ROOT).glob("*.py"), license_header("#"), verbose) | ||
| ok &= check(Path(PROJECT_ROOT).glob("*.sh"), license_header("#"), verbose) | ||
| ok &= check(PROJECT_ROOT.glob("*.go"), license_header("//"), verbose) |
There was a problem hiding this comment.
We can move this into the loop below, but maybe the current code is easier to read?
There was a problem hiding this comment.
Either is fine, but I personally prefer the current code.
scripts/license/add.py
Outdated
| add(PROJECT_ROOT.glob("*.py"), license_header("#"), verbose) | ||
| add(PROJECT_ROOT.glob("*.sh"), license_header("#"), verbose) | ||
|
|
||
| for p in PROJECT_ROOT.glob("*"): |
There was a problem hiding this comment.
nitpick: you can use iterdir instead of glob("*").
| ok &= check(Path(PROJECT_ROOT).glob("*.go"), license_header("//"), verbose) | ||
| ok &= check(Path(PROJECT_ROOT).glob("*.py"), license_header("#"), verbose) | ||
| ok &= check(Path(PROJECT_ROOT).glob("*.sh"), license_header("#"), verbose) | ||
| ok &= check(PROJECT_ROOT.glob("*.go"), license_header("//"), verbose) |
There was a problem hiding this comment.
Either is fine, but I personally prefer the current code.
ordovicia
added
the
infra
I found the current license scripts don't work if we have other top directories like
cmdbecause the patter matching*[!vendor]is not correct syntax for the expected behavior of excludingvendordirectory.