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

chore: experiment with swift-format for PR #174

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

FL33TW00D
Copy link
Collaborator

swift-format seems quite heavy handed.

@DePasqualeOrg
Copy link
Contributor

@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
Copy link
Collaborator Author

@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
Copy link
Contributor

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
Copy link
Collaborator Author

FL33TW00D commented Feb 13, 2025

@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

@FL33TW00D FL33TW00D marked this pull request as ready for review February 13, 2025 10:56
@FL33TW00D FL33TW00D requested a review from pcuenca February 13, 2025 10:56
@DePasqualeOrg
Copy link
Contributor

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
Copy link
Collaborator Author

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!

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