-
-
Notifications
You must be signed in to change notification settings - Fork 767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Completions for CLI #1119
Comments
fwiw, I don't think |
Yeah I guess it would require the repos to be cloned to get the description, and finding them and fetching them. Would need to be cached, not worth the effort unless pre-commit starts to be used as a task runner more... Surely yaml parsing of the relatively small I just ran in the pre-commit repo:
So 8ms, fast enough? I guess a |
Not sure how you're getting 8ms, even just importing pyyaml for me takes 80+ ms -- with the loading it's consistently around 90ms: $ time ./venv/bin/python -c 'import yaml; yaml.load(open(".pre-commit-config.yaml"), Loader=yaml.CSafeLoader)'
real 0m0.083s
user 0m0.070s
sys 0m0.013s |
I believe your time output is showing 100ms btw ;) ( |
Sorry typo (missed the 0), 80ms was what I got on my mac not the dev machine which i used to copy that output. That's what I was asking was fast enough, which it probably is, just about. |
I think having checks auto-completable is the only actual reason for implementing autocomplete. I love autocomplete, but pre-commit has very small number of options and it's easy to learn them, where the number of checks might vary and I never remember the names of checks we have in our repos Also pre-commit has no way to see the list of checks from command line ( I am happy to contribute it maybe even soon if we could get to agreement that this is a good idea. |
This comment has been minimized.
This comment has been minimized.
@hemna it wouldn't, actually -- click suffers from the same problem that is blocking this from completing -- please read the thread and stay on topic, thanks |
I disagree: I’m lazy and would like to type |
The |
So possible baby steps to see this happen:
There’s btw. a bunch of autocompleters that cache their results. The last step could be inspired by one of them to avoid re-parsing YAML each time. @asottile would you accept PRs implementing these? @potiuk offered help. |
Yep fully agree with sequence and caching. Caching will work - we can even cache it in the same place where pre-commit virtualenvs are cached. |
no that won't work, the pre-commit cache is shared and the data you'd be caching would be repository-specific |
Ah I see, I have not looked in detal. I thought each repo has it's own set of cached virtualenvs? I wonder how cross-repo caching works then - how do we know which virtualenv to use if there are the same check ids in different repos? Are there somehow mapped ? Even if it's not done now- I thought that you could have different pre-commit cache for different repos. It would be as easy as keeping it in a file which is named (for example) with hash of the absolute (canonical) path of the repository. |
installation is never based on the repository under test -- only the contents of the |
Then using hash of the absolute canonical path of the pre-commit.yml file should work for auto-complete cache. |
that'd be a hack at best -- while collisions are unlikely it would be very difficult to inspect and debug the actual contents usually completion engines store cached data in memory |
Then we can store it in a file named by absolute, canonical path of the pre-commit.yaml with
In a csv format. As debuggable as it can be, WDYT?
This statement is quite contrary to my recent research in this subject. Where do you get the "usually" from I wonder? Could you state your sources please? I've been looking at it recently (we change auto-complete behaviour for Airflow) and at least from my research it's not the case in Python programs (and the speed of autocompletion is not a problem in many of the programs I looked at). Usually they do it by running the auto-completable program with a _COMP variable set and take the output of that program - all the |
they may be fast enough for you but I don't consider a 100-200ms pause acceptable they also aren't cached at all and compute everything every time |
I know it is, you wrote it - that's why I proposed to implement cache (so do it diferently).
Precisely that's why I proposed cache Is the proposal above good enough? Should be fasst (<30 ms for sure) and debuggable (all plain text). Any other concerns? |
I would rather not persist anything to disk if possible -- it should be easy to store the computed state in shell variables (this is how the intermediate |
Shell variables do not take into account on-the-flght changes. And they are not faster than running a program and reading a cache from disk. I have a slight feeling you are trying to overengieer things or mayb even don't like the idea at all - but it's up to you. It's a bit of your problem not mine :), I will likely leave it to others. Just want you to be aware of the side effect of lack of autocomplete by pre-commit. We already have our own solution in Airlfow - we simply run pre-commit to prepare a python list that we load in click-based The side effect of it that our contributors don't even know that they use pre-commit as both instllatton and execution of it is hidden from most of them. Lack of autocomplete was the main reason why we decided to do that actually. So yeah I think lack of autocomplete has a side effect people won't even know they use pre-commit. But, well, your call :). |
neither of these statements are correct -- the same "on-the-flight" things can be acted on independent of where cache storage is. additionally, accessing in-memory variables is much much faster than starting a subprocess and parsing files. the disk "state" is not necessarily about speed but about complexity -- caches after all are one of the "hard" problems of CS and if there's no need to persist to disk then avoid it. and re: airflow -- cool, you're free to write whatever wrappers you want especially if they save you time. admittedly airflow's usage of pre-commit is a bit non-idiomatic (hundreds of checks) so I suspect you have needs outside the traditional ones of 99% of consumers of the tool. |
Sure. I don't really need to ask for permission on that since this is all OSS and permissve licences. I just wanted you to know that we have about ~ 2000 contributors in airflow and I run personally about 10 (20+ people each) first time contributor's workshops where I tell them how to contribute (and plan 2 more the coming months) , I could have taught them about awesomness of pre-commit (which i really believe in) - but well, out of necessity I will not - simply to avoid the confusion, I will just tell them "use breeze static-check" instead of "use pre-commit". But again - those are your product decisions. Just be aware of the results of the (potentially) over-engineering stuff rather than just doing it. But well, it is a bit funny when you want to optimize for some somethiing that 99% of your users won't even notice. - if 99% of your users are not even close to Airlfow's number of precommit's, I hardly imagine they would even notice parsing much smaller fiiles than airflow has and for sure even for airflow when they are cached. Since we are using CS quotes - I've been always tought (at my CS classes) that "premature optimization is the root of all evil", and (this is purely my own assesment that you of course have the full right to agree with or not) I believe the "in-memory" cache discussion is pure embodiment of. Multi-platform compatible in memory caching for a simple command line tool where you have to read and parse max few k text files - seems like extreme overengineering to me. There is also another saying "better done than perfect" :). No hard-feelings really - seems that we have just different understanding of the problem, and I am not going to spend any more time on it, just wanted to help others who suffer from similar problems, but seems it's not too welcome, so I will simply let it go. |
I don't want something that sucks is all. press tab in docker compose (similar size and same config language). yaml is not fast to parse (even just importing pyyaml takes 100ms) and plain variable lookups are much simpler than inventing a serialization format and a parser in shell so if you're going to talk over engineering... I don't even know why you're arguing about implementation if you have no investment in implementation or maintenance |
Trying to help others suffering from the same problem. I wrote it above. That's about it. |
I can think of two paths forward:
Regarding maintenance, I would guess that investigating and re-using the most common approach of the supported shells would make sense: Coding up custom shell code in multiple shell languages doesn’t sound fun. |
none of the existing tools do this properly unfortunately -- they call the binary every time |
If going for 1., by making the binary do the caching itself, that’d be a great solution: the implementation can be done almost exclusively in Python instead of being repeated in 3+ more or less subtly different shell languages. |
Very much agree @flying-sheep. This would be my choice as well. I think it is quite "proper" way of doing it when you consider how much things have improved since GIT implementation was around (especially with disk access time and SDD). I thinnk any "performance" concerns regarding disk are completely unfounded looking at the real datapoints.
So I really think trying to implement in-memory caching is quite not needed. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
that's cool that you think that but I do not |
Following up on #1118, it would be great to have cross-shell completions for the CLI.
These could be statically autogenerated from the output of --help options.
Would be great to have
pre-commit install-completions --shell zsh
).The text was updated successfully, but these errors were encountered: