chore: experiment with swift-format for PR

[FL33TW00D]

swift-format seems quite heavy handed.

[DePasqualeOrg]

@FL33TW00D, maybe try a line length of 120, which might result in fewer changes and looks better to my eye. If it's making too many changes that are not to your taste, it might be because it's applying default settings that you can override by specifying rules.

[FL33TW00D]

@DePasqualeOrg if you have any suggestions on config to try and minimize changes that would be great! The main one for me is how to disable adding public everywhere, Apple that is not a formatter.

[DePasqualeOrg]

It looks like the public additions are happening on extensions, and I don't see an option to avoid that in the docs. I guess it's for clarity on things that are already public.

I don't care too much about specific rules, as long as they don't make the code harder to read. I find the 100-character line length makes the code less readable, and I have my configuration files set to 120 for that reason.

[FL33TW00D]

@DePasqualeOrg Updated to 120 line length.

This can be tested locally with act pull_request -W .github/workflows/format.yml -P macos-latest=-self-hosted. Unfortunately it will still require large changes

| ❌ The following files need formatting:
| ./Tests/TensorUtilsTests/WeightsTests.swift
| ./Tests/TensorUtilsTests/LogitsWarperTests.swift
| ./Tests/TensorUtilsTests/TestUtils.swift
| ./Tests/TensorUtilsTests/TensorUtilsTests.swift
| ./Tests/HubTests/HubApiTests.swift
| ./Tests/HubTests/HubTests.swift
| ./Tests/PreTokenizerTests/PreTokenizerTests.swift
| ./Tests/PostProcessorTests/PostProcessorTests.swift
| ./Tests/NormalizerTests/NormalizerTests.swift
| ./Tests/TokenizersTests/TrieTests.swift
| ./Tests/TokenizersTests/DecoderTests.swift
| ./Tests/TokenizersTests/ChatTemplateTests.swift
| ./Tests/TokenizersTests/BertTokenizerTests.swift
| ./Tests/TokenizersTests/FactoryTests.swift
| ./Tests/TokenizersTests/TokenizerTests.swift
| ./Tests/TokenizersTests/SquadDataset.swift
| ./Tests/TokenizersTests/AddedTokensTests.swift
| ./Tests/TokenizersTests/SplitTests.swift
| ./Package.swift
| ./Sources/Tokenizers/Decoder.swift
| ./Sources/Tokenizers/UnigramTokenizer.swift
| ./Sources/Tokenizers/ByteEncoder.swift
| ./Sources/Tokenizers/Tokenizer.swift
| ./Sources/Tokenizers/PostProcessor.swift
| ./Sources/Tokenizers/BertTokenizer.swift
| ./Sources/Tokenizers/BPETokenizer.swift
| ./Sources/Tokenizers/Trie.swift
| ./Sources/Tokenizers/TokenLattice.swift
| ./Sources/Tokenizers/Normalizer.swift
| ./Sources/Tokenizers/Utils.swift
| ./Sources/Tokenizers/PreTokenizer.swift
| ./Sources/TransformersCLI/main.swift
| ./Sources/HubCLI/HubCLI.swift
| ./Sources/Models/LanguageModel.swift
| ./Sources/Models/LanguageModelTypes.swift
| ./Sources/Hub/Downloader.swift
| ./Sources/Hub/HubApi.swift
| ./Sources/Hub/Hub.swift
| ./Sources/Generation/GenerationConfig.swift
| ./Sources/Generation/Generation.swift
| ./Sources/TensorUtils/MLShapedArray+Utils.swift
| ./Sources/TensorUtils/Math.swift
| ./Sources/TensorUtils/LogitsWarper/TemperatureLogitsWarper.swift
| ./Sources/TensorUtils/LogitsWarper/LogitsWarper.swift
| ./Sources/TensorUtils/LogitsWarper/LogitsProcessor.swift
| ./Sources/TensorUtils/LogitsWarper/TopPLogitsWarper.swift
| ./Sources/TensorUtils/LogitsWarper/RepetitionPenaltyWarper.swift
| ./Sources/TensorUtils/LogitsWarper/TopKLogitsWarper.swift
| ./Sources/TensorUtils/MLMultiArray+Utils.swift
| ./Sources/TensorUtils/Weights.swift

[DePasqualeOrg]

Of course the best time to start using formatting would have been at the beginning of the project, to avoid having changes later on. But I think the benefits of having automated formatting outweigh any drawbacks (that I'm aware of) of this one-time change.

[FL33TW00D]

After discussions with @ZachNagengast, it seems that nicklockwoods swiftformat is probably a better choice than Apples swift-format.
I've updated the action accordingly.

I'd be happy to format the whole repo using this, but will defer to @pcuenca for confirmation.

Thanks @ZachNagengast!

[shavit]

If finalized, consider adding a .git-blame-ignore-revs file with this commit.

[DePasqualeOrg]

This issue has been in discussion for months, and the indecision continues to create problems for me as I try to contribute to this package. I would really appreciate it if you could decide on a solution and merge it.

[FL33TW00D]

Sorry for the delay @DePasqualeOrg

@pcuenca and myself will make sure some formatting solution is in by the end of the week.

[FL33TW00D]

If finalized, consider adding a .git-blame-ignore-revs file with this commit.

Good suggestion thank you!

[FL33TW00D]

Overall I think we've decided to go with a gated CI approach using swift-format from Nick Lockwood. I think this approach is superior to a pre-commit hook approach as it ensures consistency, and we can reduce the downside of back and forth by having the CI comment on the PR with instructions to fix the formatting issues.

The suggestion from @shavit is solid, and we can add a note about the .git-blame-ignore-revs in the README.

If @pcuenca agrees then hopefully we can go for it!

[pcuenca]

Yes, CI formatting is enough imo, we can also provide a local script to run the steps locally if we need to.

Can we have all the changes applied here so we can see what the result looks like?

[pcuenca]

We already found:

• A compilation problem because one of the rules (using keypaths) should not have been applied.
• Blank space changes in literal Strings 😱

I hope this is useful to others, but it sure is scarier than it should (and a time sink already).

[DePasqualeOrg]

Are you referring to the multi-line string in testSuccessfulDownload? That was pretty easy to resolve. Multi-line strings can be tricky, but now we can be sure that they will be correct.

[FL33TW00D]

Are you referring to the multi-line string in testSuccessfulDownload? That was pretty easy to resolve. Multi-line strings can be tricky, but now we can be sure that they will be correct.

nicklockwood/SwiftFormat#1993