search

tweag / nickel

Better configuration for less
https://nickel-lang.org/
MIT License
2.23k stars 85 forks source link

Don't blacklist files forever in the lsp #1973

Closed thufschmitt closed 1 week ago

thufschmitt commented 1 week ago

When the evaluator in the LSP times out or overflows its stack, the faulty file is blacklisted so that it doesn't get evaluated any more. This blacklist used to be permanent, which is generally not desirable since it makes the LSP unusable.

Change that to only blacklist the file for a fixed delay currently (hardcoded at 30s).

I didn't add a test, because I'm not sure how to do that properly (I assume that a test that waits for 30s isn't very desirable). Any hint at how I could do that is welcome.

thufschmitt commented 1 week ago

one solution would be to make the blacklist duration some kind of parameters instead of a static constant, that could set to a reasonable value in tests. Though we don't really have any form of config right now for the LSP, so that would probably lead to a larger change.

I though about that indeed (also for the eval timeout that might need to be bumped). Maybe a set of environment variables (like NICKEL_LSP_EVAL_TIMEOUT_SECONDS) could be a decent poor-man solution?

yannham commented 1 week ago

I think I would prefer a proper config solution - a value that is stored and accessible in the LSP somewhere in a structure (then it can be filled from an environment variable, but it's not necessarily simpler than a CL arg, and in fact we can have both such as for NICKEL_IMPORT). The testing motivation doesn't seem important enough to mandate a temporary poor-man solution, IMHO (and I suspect the full solution isn't very hard either).

thufschmitt commented 1 week ago

Well, I got a bit carried away: https://github.com/tweag/nickel/pull/1974