Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

helpers/factory: default formatter valid exit code to 0 #221

Merged
merged 9 commits into from
Jan 7, 2025

Conversation

barrett-ruth
Copy link
Collaborator

@barrett-ruth barrett-ruth commented Dec 19, 2024

Resolves #216.

Warns on formatter failures, defaulting to != 0.

Create a utility wrapper helpers.check_exit_code to maintain backwards compatibility.

TODO

  1. logger:warn spawns a blocking notification (i.e. must press enter) and creates a timeout error if not pressed within the defualt timeout

^ this is unavoidable - addressed by using a shorter initial message pointing you to to the log file.

@barrett-ruth barrett-ruth marked this pull request as draft December 19, 2024 21:09
@barrett-ruth
Copy link
Collaborator Author

@mochaaP I can't assign reviewers so please take a look. I know I need to squash commits and a bunch of other stuff but I just wanted to draft the initial API. Let me know everything that can be improved.

@mochaaP mochaaP self-assigned this Dec 19, 2024
@mochaaP mochaaP self-requested a review December 19, 2024 22:44
@barrett-ruth barrett-ruth marked this pull request as ready for review December 22, 2024 18:41
@barrett-ruth barrett-ruth marked this pull request as draft December 22, 2024 18:42
@barrett-ruth
Copy link
Collaborator Author

@mochaaP I'm going to start going through the formatters one-by-one (pain) to ensure I'm passing the right exit code.

  1. Is there a better way to do this?
  2. I currently make it optional (and default to exit code 0) to indicate formatter success in check_exit_code. Is this fine?

@mochaaP
Copy link
Member

mochaaP commented Dec 22, 2024

I think (2) is fine.

For (1), we could land the changes first and wait for feedback from ones who actually use them if any problem arises.

@barrett-ruth barrett-ruth marked this pull request as ready for review December 22, 2024 19:27
@barrett-ruth barrett-ruth marked this pull request as draft December 22, 2024 19:30
@barrett-ruth
Copy link
Collaborator Author

Ready for review.

@barrett-ruth barrett-ruth marked this pull request as ready for review December 22, 2024 19:36
lua/null-ls/helpers/formatter_factory.lua Outdated Show resolved Hide resolved
lua/null-ls/helpers/check_exit_code.lua Outdated Show resolved Hide resolved
lua/null-ls/helpers/formatter_factory.lua Show resolved Hide resolved
@barrett-ruth
Copy link
Collaborator Author

barrett-ruth commented Dec 28, 2024

@mochaaP addressing both comments above:

The problem I have with moving the code to the generic factory (and this includes the code duplication comment above) is that I thought this feature only makes sense for formatters.

If we added this to generator factory, wouldn't we also be checking linting exit codes as well?

@barrett-ruth
Copy link
Collaborator Author

@mochaaP this is ready for review, see above comments

@barrett-ruth
Copy link
Collaborator Author

What does the eyes emoji mean 😭 I've already addressed the comments I thought... what do you think? (lol)

@mochaaP
Copy link
Member

mochaaP commented Jan 5, 2025

“I will take a look at it” 😂 This needs some slight changes, I will take care of it.

@mochaaP
Copy link
Member

mochaaP commented Jan 5, 2025

Do you think this implementation is okay?

@barrett-ruth
Copy link
Collaborator Author

@mochaaP you updated the logging message to use logger:add_entry.

Then you deleted the file containing that change (check_exit_code) - lol! I'm wondering if this was intentional? I thought we wanted this informative message to the end user?

If this is the case, I thought we had to separate the logic out in another file to do that log... because only in the formatters does the notion of validating the exit code make sense.

Sorry this is taking so long, LMK if you know what I'm saying.

@mochaaP
Copy link
Member

mochaaP commented Jan 5, 2025

ack, will check tonight.

@barrett-ruth
Copy link
Collaborator Author

LGTM!

@mochaaP mochaaP changed the title Validate Formatter Exit Codes helpers/factory: default formatter valid exit code to 0 Jan 7, 2025
@mochaaP mochaaP merged commit cc810f7 into nvimtools:main Jan 7, 2025
3 checks passed
barrett-ruth added a commit to barrett-ruth/none-ls.nvim that referenced this pull request Jan 7, 2025
barrett-ruth added a commit that referenced this pull request Jan 7, 2025
@geril2207
Copy link

@mochaaP @barrett-ruth
prettierd nvim(nightly but idk if it changes anything):

2025-01-09.12-19-02.mp4

before:

2025-01-09.12-19-43.mp4

@mochaaP
Copy link
Member

mochaaP commented Jan 9, 2025

cc @barrett-ruth is prettierd outputting to stdout? we might need to ignore that

@daephx
Copy link

daephx commented Jan 10, 2025

Found myself here because I updated none-ls and keep getting the error nvim_echo must not be called in a lua loop callback when the buffer has invalid syntax. I've only tested with stylua, and haven't checked other formatters.

The error output itself is rather annoying, so for now my solution is to pin none-ls to commit 83c7c00.

Edit: Ok, I just spotted #237 which has the same changes I concluded on after exploring the traceback.

@mochaaP
Copy link
Member

mochaaP commented Jan 11, 2025

I've partially reverted this until I figure out a better way to present the error message.

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.

Formatting: Show Error on failure
4 participants