apple-swift-format linter & fixer

[bosr]

Hi, thanks for ALE, it is awesome!

In this PR I propose a name change for apple/swift-format from swift-format to apple-swift-format. For instance, the global variable for the executable becomes g:ale_swift_appleswiftformat_executable.

• I rename (and refactor a little bit) the existing linter support & tests by @klaaspieter
• I add the apple-swift-format fixer and its test.
• Both the linter and the fixer can leverage the g:ale_swift_appleswiftformat_use_swiftpm (for users using swift-format as their project dependency).

I hope you will agree with this naming change, as it distinguishes clearly in ALE the SwiftFormat (the community supported project) and the newer apple/swift-format project, enabling both projects to have their own linter and fixer.

Before this PR the status for swift-format was a bit unfortunate:

• apple/swift-format has a linter, but no fixer
• SwiftFormat has no linter, but a fixer.
• naming confusion
• no promotion in doc/ale-swift.vim

I hope this is now improved.

FYI @w0rp @klaaspieter (#3007) @WhyNotHugo (#3462)

Cheers

[bosr]

I'll fix the failing test, my bad. I did not run vader locally...

[bosr]

In the Vader linting tests for apple-swift-format, I'm getting [EXECUTE] (X) No linters were loaded. I'm missing something, could you provide a pointer/hint? Thanks!

Edit: fixed

[bosr]

I think the PR is now ready.

[hsanson]

I do think the proposed change is very good but also is important to consider that there may be users that already use this tool with the g:swift_swiftformat_executable configuration. Changing this is a breaking change that goes against ALE philosophy:

Never introduce any breaking changes ever, unless users have been previously warned about them.

We cannot remove g:swift_swiftformat_executable variable so both must be supported giving priority to g:swift_apple_swift_format_executable. And in the documentation we mention that g:swift_swiftformat_executable is deprecated by g:swift_apple_swift_format_executable. Also ensure there are tests for falling back to g:swift_swiftformat_executable if defined.

Finally create an issue to completely remove g:swift_swiftformat_executable from the code after a while. Maybe one or two releases in the future.

[hsanson]

This function is exactly the same used in the linter. Better move it to autoload/ale/swift.vim file to keep the code DRY.

[bosr]

Hi, thanks for the review! I will happily apply the requested changes.

One clarification though: I think swiftformat should not be deprecated; on the contrary it should become available for the community-supported SwiftFormat project to bring linting support in ALE: today, it is quite hard for them because the name is already taken and the CLI is not compatible. Ideally, #3007 should have used a different name. Just as a reminder, the SwiftFormat project has a fixer in fixers/swiftformat.vim.

Back to your point on ALE philosophy (with which I agree), we need a migration path which does not break existing functionality. Here are the steps I have in mind:

• apply the requested naming changes above
• hold on factorizing the code in autoload/ale/swift.vim (because it would only help apple/swift-format and be incompatible with SwiftFormat)
• re-instate the original support for apple/swift-format in ale_linters/swift/swiftformat.vim but also provide basic support for SwiftFormat in the same file, as follows:
  • introduce a new dictionary (or string, not yet sure) variable g:ale_swift_swiftformat_options (empty by default) to provide CLI options to SwiftFormat
  • modify the GetExecutable function to first locate the swiftformat binary (from SwiftFormat), then fallback on searching for the swift-format binary (from apple/swift-format). If swiftformat is found, the options dictionary is used, if swift-format is found the existing behavior (I mean from Add Apple's swift-format as a linter #3007) is maintained. This has to be documented in the doc/ale-swift.txt file.
• as you said, in a few releases, drop support for apple/swift-format in the ale_linters/swift/swiftformat.vim and only keep it in ale_linters/swift/apple_swift_format.vim

I hope this sounds like a plan to solve this. I'll wait for your feedback to start coding.

[hsanson]

@bosr I think I understand better what the issue is and what you are trying to do.

The fact that both SwiftFormat (Fixer) and Apple Swift Format (Linter) use the same variable name g:swift_swiftformat_executable to set their executable is unfortunate indeed. I think in this case the breaking change is necessary and can be tolerated.

When refactoring ale_linters#swift#appleswiftformat#GetExecutable() into ale/swift.vim, please name it something like ale#swift#GetAppleSwiftFormatExecutable(). This way in the future we can add ale#swift#GetSwiftFormatExecutable() for the other tool that can also be used as both linter and fixer.

[bosr]

@hsanson, before I make the changes, do you have a preference for the variable name?

g:ale_swift_appleswiftformat_use_swiftpm or g:ale_swift_apple_swift_format_use_swiftpm

In retrospect, I find the former to be more readable and aligned with others like g:ale_swift_sourcekitlsp.

[hsanson]

@bosr I have no strong preference so please use whichever feels best for you that actually use this tool.

I do like to take the official tool name and replace the "/" and "-" chars with "_" to make the variables:

• apple/swift-format becomes g:ale_swift_apple_swift_format_xxx
• SwiftFormat becomes g:ale_swift_swiftformat_xxx
• apple/sourcekit-lsp should have been g:ale_swift_apple_sourcekit_lsp_xxx
• etc...

[bosr]

Hi @hsanson, I made some progress factoring the code in autoload, but I have a regression on the fixer tests which I cannot fix.

Starting Vader: /testplugin/test/fixers/test_appleswiftformat_fixer_callback.vader
    (1/2) [EXECUTE] The swiftformat callback should return the correct default values
    (1/2) [EXECUTE] (X) Vim(function):E127: Cannot redefine function ale#fixers#appleswiftformat#Fix: It is in use
      > /testplugin/autoload/ale/fixers/appleswiftformat.vim, line 16
    (2/2) [EXECUTE] The swiftformat callback should use swiftpm is use_swiftpm is set to 1
    (2/2) [EXECUTE] (X) Vim(function):E127: Cannot redefine function ale#fixers#appleswiftformat#Fix: It is in use
      > /testplugin/autoload/ale/fixers/appleswiftformat.vim, line 16
  Success/Total: 0/2

I've spent hours trying to fix it, so if you could help me, that would be great.

Thanks

[hsanson]

@bosr this method is being used but does not seem to be defined anywhere: ale#fixers#appleswiftformat#GetArguments() I would start by defining it and see if this fixes that "cannot redefine" error.

[bosr]

Thanks a lot, that was right under my nose 🤦‍♂️.

Now all tests pass, the names could be changed, but if it up to me, I like them as they are because they are readable and match with other name.

[bosr]

Note: I have some issues using it on my local machine, so I need more time to debug before merging. Thanks.

[bosr]

Hello @hsanson, my local problems are solved, tests pass, and the local behavior is correct for the linter and fixer, with/without use_swiftpm, and with/without a .swift-format.

I would like to add one last test for the .swift-format config file. I know the behavior is correct locally: meaning ALE uses the available .swift-format from my project folder for both the linter and the fixer, but in the Vader tests I could not manage to use correctly the SetDirectory and SetFilename for the .swift-format to be located correctly and the --configuration /path/to/.swift-format argument to be passed in the test.

This test is commented in test/linter/test_swift_appleswiftformat.vader). Could you provide guidance on this? Thanks!

[hsanson]

@bosr please check comments for tips on fixing the test.

[bosr]

Thanks for the hints!

FYI, my last updates were about passing the tests with --configuration on windows.

I think I'm good now.

[hsanson]

Thanks for all the work and patience.

[bosr]

My pleasure! Thank you for the patience and guidance. It was much more laborious than expected, but thanks to you I learned a lot.