google / vim-codefmt

Vim plugin for syntax-aware code formatting
Apache License 2.0
1.11k stars 114 forks source link

`ktfmt` availability check is broken (on Linux) #218

Closed grodtron closed 1 year ago

grodtron commented 1 year ago

Describe the bug When trying to use ktfmt I get the following error:

Formatter "ktfmt" is not available. Setup instructions: Install ktfmt (https://github.com/facebookincubator/ktfmt). Enable with                                                                                                                                                          
Glaive codefmt ktfmt_executable=java,-jar,/path/to/ktfmt-<VERSION>-jar-with-dependencies.jar   

However, if I reverse the conditional check like this then everything works fine. I feel like I may be missing something since I would have thought others would encounter this issue, but I have tested this locally and it fixes the issue for me. If you agree that this change makes sense I can sign the CLA and make a pull request, or feel free to just make the (one character) change directly.

To Reproduce Minimal vimrc snippet, does not include installing codefmt itself:

Glaive codefmt                                                                                                                                                                                                                                                                           
    \ plugin[mappings]=',='                                                                                                                                                                                                                                                              
    \ ktfmt_executable=java,-jar,/local/home/grodtron/.tools/ktfmt-0.44-jar-with-dependencies.jar                                                                                                                                                                                        

" Auto format on buffer write                                                                                                                                                                                                                                                            
augroup autoformat                                                                                                                                                                                                                                                                       
    autocmd!                                                                                                                                                                                                                                                                             
    autocmd FileType kotlin AutoFormatBuffer ktfmt                                                                                                                                                                                                                                       
augroup END                           

Steps to reproduce the behavior:

  1. Open any kotlin file
  2. Write the file (:w)

Expected behavior The file should be formatted

OS (version) Linux

Additional context n/a

dbarnett commented 1 year ago

How strange!

I'm okay with just reversing that check, may regress for some cases but if so they were always working for the wrong reasons, and we could take those issues in stride. The ideal would be also having the check analyze the exit code and stderr to distinguish straightforward "ktfmt not available" cases from other errors, and logging a more detailed warning with actual errors from command in that case (so if we do regress for someone, we won't have to go hunting for more repro details to understand how to fix it on their system).

grodtron commented 1 year ago

Cool, I am happy to submit a PR for this in that case, and add that additional logging 👍

grodtron commented 1 year ago

I spent some more time looking into this, and it seems that the failure cases actually always already end up in the catch block of the try catch, where a nice error is already logged. A couple examples:

ktfmt unavailable, check jar file in `java -jar foobar.jar -`: ERROR(ShellError): Error running: java -jar foobar.jar -                                       
Error: Unable to access jarfile foobar.jar
ktfmt unavailable, check jar file in `java -xx -`: ERROR(ShellError): Error running: java -xx -                                                               
Unrecognized option: -xx                                                                                                                                      
Error: Could not create the Java Virtual Machine.                                                                                                             
Error: A fatal exception has occurred. Program will exit.  

Since there's already decent error messages, I am just submitting my change directly as a CR - but please let me know if you'd like any modifications to it!