chore(lint): lint TablePro's own SwiftPM packages - #2882
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.swiftlint.ymlreadincluded: [TablePro]withPackagesinexcluded:, so TablePro's own SwiftPM packages, 362 Swift files, had never been linted. This brings them in and clears the 253 violations that were waiting there.The count is not the point. Turning the gate on found three real defects, one of them user-visible under test, and they are the reason this change is worth its size.
Sixth step of the editor adoption work, after #2873, #2875, #2877, #2878, #2880 and #2881. The two vendored editor forks stay outside
included:for now; they join when they move intoPackages/and stop being forks.The config
.buildhas to be globbed. SwiftLint resolves anexcluded:entry against the config file's own directory, so the bare.buildthat was there matches<root>/.buildand nothing else. WithPackagesincluded and the old entry left alone, a machine that has resolved its dependencies reports 41,408 violations, 41,156 of them insidePackages/*/.build/checkoutsin oracle-nio, BigInt and swift-nio. CI never sees them because its checkout has no resolved packages, so the rule would have passed in CI and failed on every developer's machine, which is the worst shape a lint rule can have.What the gate found
The sync record cache wrote into the developer's own Application Support under a UI test.
SyncCoordinatorbuiltSyncRecordCache()with no directory, and the package resolved one itself from.applicationSupportDirectory, bypassingAppStorageEnvironment. That is exactly what the repo's ownstorage_environment_directoryrule exists to catch, and the rule had never run overPackages/. A package cannot import the app target, so it cannot askAppStorageEnvironmentanything: the fix is that it no longer guesses.directoryanddefaultsare required parameters now, and the app passesAppStorageEnvironment.shared.supportDirectoryand.defaults. The production path is byte-identical to the old one; only the isolated case changes, which is the case that was broken.A Teradata test was passing on replacement characters. Clearing
optional_data_string_conversioninTeradataTLSTeststurnedString(decoding:as: UTF8.self)into the failable initializer and the test went red. The client-attributes parcel body is a binary TLV buffer (big-endianu16code,u16length, a0xBFcharset byte), so it is not valid UTF-8 and never was; the lossy initializer had been substituting U+FFFD to makecontains("SSLM=REQUIRE")match. It now decodes through.isoLatin1, which maps all 256 byte values one to one and preserves the ASCII markers the assertion looks for.Two
swiftlint:disable force_unwrappingdirectives inAnalyticsHeartbeatServicesuppressed nothing. They were presumably right once, the code moved, and nothing said so.superfluous_disable_commandis the rule that notices, and it had never run here.swiftlint --fixbroke a file, and the app build did not catch itempty_countrewrotereplaced.count > 0into!replaced.isEmptyinMSSQLParameterBatch, wherereplacedis a tuple(query: String, count: Int), not a collection.TableProMSSQLCorestopped compiling.The app build still passed, because the app scheme does not compile that target: only
AllPluginsdoes. Anything that runsswiftlint --fixover a package has to compile the package afterwards, not just the app.Fixed by renaming the tuple's field to
placeholderCount, which is what it counts and which a reader will not mistake for a collection either.Directives
Five, each because the rule is wrong for that line, never to go green:
DHVectors.swift,line_length. A generated table of RFC 3526 Group 14 test vectors whose banner says "Do not edit". Each violating line is one hex literal of 512 bits or more. Wrapping it changes the value, shortening it changes the vector, and the vector is the test.SQLiteMasterQueries,inclusive_language.sqlite_masteris SQLite's own catalog table. Renaming the type would make it lie about what it queries.String(localized:)of 225 characters inAWSAuthError,line_length. Splitting the literal changes the catalog key.R2SQLResponseDecoder.snippet,optional_data_string_conversion. It shows the reader the body of a response that failed to decode. Lenient decoding is the point: a failable initializer answers nil for exactly the malformed body worth seeing.RecordDecoder,optional_data_string_conversion, on the Teradata CHAR/VARCHAR cell path. See below.A directive has to sit on the violating line, not above the declaration: putting one between a doc comment and its declaration produces
orphaned_doc_comment, and putting one above afuncdoes not cover a violation inside it.Left for its own change
RecordDecoderdecodes every Teradata text cell as UTF-8 with lossy substitution. The failable initializer would drop the cell instead, which is worse, so neither is right: the cell should decode by its column's own character set, the wayMySQLColumnDecodingdoes after #2725. That is a driver correctness change with its own tests and does not belong in a lint commit.Also in this diff
ProtocolTests.swiftandTrinoWriteSQLTests.swifteach held severalXCTestCasesubclasses and are split into one file per class, named after the class. Every test method moved verbatim.WelcomeOutlineRows.swiftin the app target:aspectRatio(contentMode: .fit)toscaledToFit(). The violation is pre-existing onmainand CI's older SwiftLint does not report it, so it only shows locally. Without itswiftlint lint --strictcannot pass on a developer's machine, which is what this change is for.Packages/TableProCore/Sources/TableProPluginKitis a symlink toPlugins/TableProPluginKit/, so includingPackagesalso brings PluginKit under lint for the first time. Seven violations there, none of them an API change:first(...) != nilbecameconstraints.contains(where:)on the public array that was already there, rather than adding a method to a public type, which would have been an ABI change and acurrentPluginKitVersionbump for a style fix.Verification
swiftlint lint --strict: 0 violations acrossTablePro,Packagesand PluginKit.swift test --package-path Packages/TableProCore: 289 cases across 8 test targets, 0 failures.AllPluginsfails on this machine with oracle-nio's@TaskLocalmacro expanding tounknown attribute 'usableFromInlinenonisolated'under Xcode 27 / Swift 6.4. That is the documented pre-existing failure, and it is the only error in the log: zero errors come from this diff. CI's Xcode 26.4.1 is where that target actually compiles.scripts/check-pluginkit-abi.sh origin/main: see the checks.