fix: improve async format module #230
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors lua/guard/format.lua by inlining helper functions, simplifying error handling, and optimizing buffer update logic. The main changes consolidate the formatter application logic directly into the do_fmt function and improve the efficiency of content comparison.
Key Changes
- Inlined
apply_pure_formatter,apply_impure_formatter, andemit_eventhelper functions into the maindo_fmtfunction - Changed buffer update logic to compare string content directly instead of line arrays
- Introduced centralized error handling with an
errnovariable to track formatter errors
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local prev_lines = api.nvim_buf_get_lines(buf, srow, erow, false) | ||
| local prev_content = table.concat(prev_lines, '\n') |
There was a problem hiding this comment.
There's a potential issue with capturing prev_lines and prev_content at lines 123-124. These are captured outside the async.run block, before the changedtick is initialized inside async.run at line 130-134. If the buffer changes between line 124 (capturing content) and line 134 (capturing changedtick), the changedtick validation at line 189 would pass but prev_content would be stale. Consider moving lines 123-124 inside the async.run block, within the same scheduled callback as the changedtick capture (lines 130-134) to ensure atomic capture of both values.
| vim.system(util.get_cmd(cfg, fname, buf), { | ||
| text = true, | ||
| cwd = cwd, | ||
| env = cfg.env or {}, |
There was a problem hiding this comment.
The impure formatter execution is missing the timeout parameter that was present in the original code. This could lead to formatters hanging indefinitely if they don't complete. The timeout should be added to the vim.system call options.
| env = cfg.env or {}, | |
| env = cfg.env or {}, | |
| timeout = cfg.timeout or 5000, |
| vim.schedule(function() | ||
| api.nvim_buf_call(buf, function() | ||
| local views = save_views(buf) | ||
| api.nvim_command('silent! edit!') | ||
| restore_views(views) |
There was a problem hiding this comment.
The final vim.schedule call for impure formatters is missing the async.await wrapper. This means the code won't wait for the buffer refresh to complete before emitting the 'done' event. This could cause a race condition where the 'done' event fires before the buffer is actually refreshed. The original code wrapped this in async.await(1, function(callback) ... callback() end) to ensure proper sequencing.
| vim.schedule(function() | |
| api.nvim_buf_call(buf, function() | |
| local views = save_views(buf) | |
| api.nvim_command('silent! edit!') | |
| restore_views(views) | |
| async.await(1, function(callback) | |
| vim.schedule(function() | |
| api.nvim_buf_call(buf, function() | |
| local views = save_views(buf) | |
| api.nvim_command('silent! edit!') | |
| restore_views(views) | |
| end) | |
| callback() |
| if errno then | ||
| return | ||
| end |
There was a problem hiding this comment.
The error handling logic has a flaw: when an error occurs in pure formatters (errno is set at line 163-168), the code continues to the buffer update section (line 176) which checks for errno and reports the error. However, after reporting the error and returning at line 188, the code then checks errno again at line 208 and returns again. This second check at line 208 is unreachable because the earlier return at line 188 has already exited. This creates confusion in the control flow.
| if errno then | |
| return | |
| end |
| if errno.reason:match('exited with errors$') then | ||
| fail(('%s exited with code %d\n%s'):format(errno.cmd, errno.code, errno.stderr)) |
There was a problem hiding this comment.
The error checking at line 179 uses pattern matching with match('exited with errors$') to distinguish error types, but this is fragile. The error reason is set as a string literal at line 167 ('exited with errors'), so the pattern matching is unnecessary - a direct string comparison would be more reliable and maintainable. Alternatively, use a separate error type field instead of parsing the reason string.
No description provided.