Conversation
taplo lint supports the --cache-path argument to cache downloaded schemas. This adds the parameter if not passed already, using a directory in the activated venv.
redeboer
left a comment
redeboer
left a comment
There was a problem hiding this comment.
Looks good, but I have to check if it all works as expected. Some small comments below
| name: taplo-lint | ||
| description: Lint TOML documents | ||
| entry: taplo lint | ||
| entry: cached-taplo-lint |
There was a problem hiding this comment.
Maybe better to have a separate hook? I don't want to temper with the default behaviour of taplo-lint.
There was a problem hiding this comment.
I understand your concern, but I'd still "shadow" the original hook:
- I suspect that most users just install the hook and forget, no fancy setup, no weird customizations, no reading docs when updating => having it "magically go faster" after a
pre-commit autoupdateseems the best we can do. - Those who added
--cache-path somethingshouldn't be impacted, since the wrapper takes this case into account (and does nothing when it's the case).
Sure things could break in other ways:
taplo lintchanging the caching behavior- some Murphy laws 🤷
Of course you have the last word.
cached_taplo_lint.py
Outdated
| args = sys.argv[1:] | ||
|
|
||
| if '--cache-path' not in args: | ||
| cache_path.mkdir(parents=True, exist_ok=True) | ||
| args.extend(['--cache-path', str(cache_path)]) |
There was a problem hiding this comment.
I tried a little but I'm not really getting it.
ArgumentParser main goal seems to be checking and preparing arguments passed to the script to be used by itself. In this case, we need to construct arguments to be passed to another command, while very much not parsing anything else (taplo does that by itself already - and we're not re-implementing it all, right? 🤔)
Could you give me an example on what you are thinking about?
serl
left a comment
serl
left a comment
There was a problem hiding this comment.
Thanks for the very fast review!
(I added some simple tests to the wrapper in the meantime, just to be sure that it behaves)
| name: taplo-lint | ||
| description: Lint TOML documents | ||
| entry: taplo lint | ||
| entry: cached-taplo-lint |
There was a problem hiding this comment.
I understand your concern, but I'd still "shadow" the original hook:
- I suspect that most users just install the hook and forget, no fancy setup, no weird customizations, no reading docs when updating => having it "magically go faster" after a
pre-commit autoupdateseems the best we can do. - Those who added
--cache-path somethingshouldn't be impacted, since the wrapper takes this case into account (and does nothing when it's the case).
Sure things could break in other ways:
taplo lintchanging the caching behavior- some Murphy laws 🤷
Of course you have the last word.
cached_taplo_lint.py
Outdated
| args = sys.argv[1:] | ||
|
|
||
| if '--cache-path' not in args: | ||
| cache_path.mkdir(parents=True, exist_ok=True) | ||
| args.extend(['--cache-path', str(cache_path)]) |
There was a problem hiding this comment.
I tried a little but I'm not really getting it.
ArgumentParser main goal seems to be checking and preparing arguments passed to the script to be used by itself. In this case, we need to construct arguments to be passed to another command, while very much not parsing anything else (taplo does that by itself already - and we're not re-implementing it all, right? 🤔)
Could you give me an example on what you are thinking about?
2 participants
Closes #28
taplo lint supports the --cache-path argument to cache downloaded schemas. This adds the parameter if not passed already, using a directory in the activated venv.