Skip to content

Conversation

@one-d-wide
Copy link

Add a recipe that show cases how to display formatter output if it fails or explanation if it is unable to run.

A nice error message, similar to one below, will show up on the bottom of the screen to help quickly spot an error:

Formatter 'rustfmt' error: error: expected `;`, found `bar`
 --> <stdin>:2:10
  |
2 |     foo()
  |          ^ help: add `;` here
3 |     bar()
  |     --- unexpected token

Press ENTER or type command to continue

P.S. I think this would be nice to have enabled by default. Given that it would probably help to reduce frustration when formatter wasn't actually found/installed, or absence of formatting action wasn't immediately obvious (hint to check conform log appears only once). And this doesn't seem to anyhow worsen the experience.

@github-actions github-actions bot requested a review from stevearc June 27, 2024 19:10
@stevearc
Copy link
Owner

stevearc commented Jul 1, 2024

There are two reasons why a formatter may fail. 1 is that it's not installed, 2 is that it errored during execution. Our approach for 1 has been to detect this state and not even run the formatter if we know it's not installed; I don't believe your snippet would affect this at all. For 2, an error in execution typically means one of two things. Either
a) There is a problem with the formatter, or
b) There is a problem with the file (e.g. syntax error)

In both of these situations, there is something that the user needs to go and fix. If it's the formatter, well now you know and we shouldn't spam you with that notification every time you save a file. After all, you may be trying to get work done and not care right now about fixing the formatter. If the problem is in the file, you probably have better mechanisms to tell you about the issue (LSP, nvim-lint). Again, there's not much value to spamming you with the same notification every time you save.

Can you explain more why you think this is beneficial? It doesn't make much sense to me why you would want rustfmt to be responsible for reporting your syntax errors, when rust-analyzer is the tool intended for that job.

@one-d-wide
Copy link
Author

one-d-wide commented Jul 2, 2024

Thank you for the feedback. My main reason to propose this snippet was basically to make a formatter error more obvious, since usually a clear syntax error (or missing dependency) is far from what is expected.

a) (There is a problem with the formatter)

Currently there is no difference in feedback between formatter successfully doing its thing and no attempt at formatting taken at all, if e.g. formatter wasn't installed. It seems frustrating, at least in my view, to observe nether a formatting applied nor any error message shown after I explicitly triggered formatting.

So now there is actually one-line error being displayed: "No formatters found for buffer". It's short, so it doesn't require any acknowledgement from the user. And hence, it doesn't show up if formatting was triggered automatically, because it's immediately being overwritten by the file save notification.

b) (There is a problem in the file)

I think you made a good argument here. Though I would add that when LSP/linter is engaged, a syntax error, that would instantly fail formatting, doesn't come through and show up very often (and I didn't noticed problems with error spamming, at least yet). Or alternatively, if such a failure is to be expected, I think it is better not to guess and only disable (ignore) formatter explicitly. But I get now why this behavior may not be desirable as a default. Also I guess the error notification from the formatter still would make sense, if an underlying syntax error still slipped unnoticed, like for example if there is no LSP/linter in use.

I meant to show output of rustfmt only as an example of how notification is displayed, since it has quite pretty error messages, obviously it's not a replacement for any linter or lsp, that would produce far more detailed and useful feedback.

@stevearc
Copy link
Owner

As part of #491 I've added a default behavior of notifying the user once-per-filetype if you attempt to format but none of the configured formatters are available. Would that address your use case?

@one-d-wide
Copy link
Author

I can confirm that, with the patch applied, there's an one-time warning being shown when configured formatter can't be found. Though there's still no visible notification when formatter itself errors. If you feel like this is too specific use-case even as a customization example, feel free to close this PR.

@stevearc
Copy link
Owner

There's no notification when the formatter fails? That's very strange, can you file a bug report with a repro? It should produce a notification that says Formatter failed. See :ConformInfo for details

@one-d-wide
Copy link
Author

one-d-wide commented Jul 17, 2024

Oops, I forgot to remove notify_on_error = false. The notification you specified indeed shows up. But it doesn't display an actual error message, making you go to the logs if you want to see it. And, because of the debouncing, it's not immediately obvious whether the formatting is working again or not when you retry.

@stevearc
Copy link
Owner

I think the current notification system is good for most people. I certainly don't want to prevent you from customizing the way error handling is done, but I don't think this is a good fit for a recipe. The goal of the recipes is mostly to function as a quick link that I can paste when people file a feature request for something (e.g. "how can I disable formatting temporarily"). I don't expect this is a pattern that is going to be widely desired or requested, so I'd prefer to leave it out of the docs.

I do appreciate the drive to share your improvements, and the subsequent discussion around it that led to refactoring some of the notification logic!

@stevearc stevearc closed this Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants