search

jdugan6240 / kak-bundle

Mirror to https://codeberg.org/jdugan6240/kak-bundle. Contributions are accepted here, but they are preferred upstream.
BSD Zero Clause License
30 stars 3 forks source link

Bundle status for install / update #10

Closed mralusw closed 3 years ago

mralusw commented 3 years ago

Phew, this was quite a bit of work, but it looks really, really nice. If you have questions about the decisions in my code, let me know.

I've tried command_fifo for a while, but it has some pretty serious bugs (https://github.com/mawww/kakoune/issues/4410). Thus I switched to implementing a "periodic hook" for *bundle-status*, which turned out to be pretty simple, elegant, non-blocking for the user, and much less complex on top of that. However, having some command_fifo code in git history might be useful for the future (as well as for debugging kak); thus I didn't remove it.

bundle_parallel is now an int controlling the maximum number of parallel jobs. It had to be done, after all we don't want 100 git clone / pull's running.

jdugan6240 commented 3 years ago

This is lovely. I've been testing it out at work today, and it really works. I really like the idea of periodically populating the status buffer with the contents of the temp files - this looks great.

There is one problem, though - the temp files are no longer getting cleaned up. I suspect this is due to the fact that tmp_dir is blanked out whenever you source bundle_sh_code, and thus you try to remove a nonexistent directory. I think defining a hidden option called bundle_tmp_dir, and using that instead, may fix it, though.

The bottom line is, as excellent as this is, until the temp directory cleanup issue is solved, I can't merge this.

mralusw commented 3 years ago

There is one problem, though - the temp files are no longer getting cleaned up. I suspect this is due to the fact that tmp_dir is blanked out whenever you source bundle_sh_code, and thus you try to remove a nonexistent directory. I think defining a hidden option called bundle_tmp_dir, and using that instead, may fix it, though.

Heh. There actually is an option with that exact name and purpose :) However, you are right, it seems there is a bug which I have missed. Let me get back to you.

mralusw commented 3 years ago

OK, this was a simple fix; can you try it now please?

jdugan6240 commented 3 years ago

Much better. I'll merge this when I get home from work.