feat: support nested directory for tmpfile_format #804
Conversation
PeterCardenas
force-pushed
the
support-nested-tmpfile-format
branch
from
3a23ad7 to
ca9653f
Compare
PeterCardenas
force-pushed
the
support-nested-tmpfile-format
branch
from
ca9653f to
5a1abc9
Compare
lua/conform/runner.lua
Outdated
| if rel_dirname ~= "." then | ||
| local dirname = vim.fs.joinpath(ctx.dirname, rel_dirname) | ||
| log.debug("Cleaning up temp dir %s", dirname) | ||
| uv.fs_rmdir(dirname) | ||
| end |
There was a problem hiding this comment.
If we're going to go to the trouble to removing dangling directories, I'd like to make sure it's a thorough solution rather than a special case for if it's a single directory deep. Since we're creating the directories just above this, it would be pretty easy to do a quick check there to keep track of which ones in that path already exist, and then we can attempt to remove the set of new directories on the way out.
PeterCardenas
force-pushed
the
support-nested-tmpfile-format
branch
from
5a1abc9 to
054dc91
Compare
PeterCardenas
force-pushed
the
support-nested-tmpfile-format
branch
from
054dc91 to
4208474
Compare
PeterCardenas
force-pushed
the
support-nested-tmpfile-format
branch
from
4208474 to
36618cd
Compare
lua/conform/runner.lua
Outdated
| .iter( | ||
| vim.split( | ||
| assert( | ||
| vim.fs.relpath(ctx.dirname, ctx.filename), |
There was a problem hiding this comment.
Conform still supports Neovim 0.10, but vim.fs.relpath wasn't introduced until 0.11
lua/conform/runner.lua
Outdated
| vim.fs.relpath(ctx.dirname, ctx.filename), | ||
| "filename " .. ctx.filename .. " is not a child of dirname " .. ctx.dirname | ||
| ), | ||
| "/", |
There was a problem hiding this comment.
Splitting by / might not work on windows. Could we do this maybe procedurally or recursively using vim.fs.dirname to find the first parent dir that does exist?
lua/conform/runner.lua
Outdated
| -- If all directories relative to ctx.dirname exist, there are no directories to remove. | ||
| if can_remove then | ||
| log.debug("Cleaning up temp dir %s", dir_to_remove) | ||
| vim.fs.rm(dir_to_remove, { recursive = true }) |
There was a problem hiding this comment.
vim.fs.rm is also only in nvim 0.11
There was a problem hiding this comment.
And small potential race condition: it is possible to run multiple formatters at the same time, either with :wall or with the injected formatter. If multiple formatters use the same tmpdir it's possible that the cleanup for one will clobber the active file for another. It's best to safely remove the directories from the deepest first using fs_rmdir.
There was a problem hiding this comment.
so if we're considering race conditions, we could potentially also end up in a scenario where we still have dangling directories if we only calculate the directories to remove before formatting. alternatively we could try to remove empty directories indiscriminately up until ctx.dirname. this would solve the race condition but we might end up deleting directories that the user wanted to keep around (even though they're empty). i'll push up a commit for indiscriminately removing, but wondering your thoughts here
There was a problem hiding this comment.
Good point. I think racing to leave a dangling directory is better than racing to mangle formatting, but ideally we wouldn't do either.
What about tracking the directories to clean up in a local variable declared outside the function? So each run will add any new directories it creates and attempt to clean up all directories that have been created by any run.
PeterCardenas
force-pushed
the
support-nested-tmpfile-format
branch
2 times, most recently
from
619231e to
b2182ee
Compare
PeterCardenas
force-pushed
the
support-nested-tmpfile-format
branch
from
b2182ee to
b7c693d
Compare
2 participants
tmpfile_format with a nested directory works for the most part with the exception that the directory is left behind, leaving the file system cluttered with temporary directories. to resolve this, we remove the first directory within tmpfile_format that does not already exist.