Skip to content

chore(lint): lint TablePro's own SwiftPM packages - #2882

Merged
datlechin merged 1 commit into
mainfrom
chore/lint-first-party-packages
Sep 15, 2026
Merged

datlechin merged 1 commit into
mainfrom
chore/lint-first-party-packages

Conversation

@datlechin

Copy link
Copy Markdown
Member

Summary

.swiftlint.yml read included: [TablePro] with Packages in excluded:, 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 into Packages/ and stop being forks.

The config

included:
  - TablePro
  - Packages

excluded:
  - DerivedData
  - "**/.build"
  ...

.build has to be globbed. SwiftLint resolves an excluded: entry against the config file's own directory, so the bare .build that was there matches <root>/.build and nothing else. With Packages included and the old entry left alone, a machine that has resolved its dependencies reports 41,408 violations, 41,156 of them inside Packages/*/.build/checkouts in 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. SyncCoordinator built SyncRecordCache() with no directory, and the package resolved one itself from .applicationSupportDirectory, bypassing AppStorageEnvironment. That is exactly what the repo's own storage_environment_directory rule exists to catch, and the rule had never run over Packages/. A package cannot import the app target, so it cannot ask AppStorageEnvironment anything: the fix is that it no longer guesses. directory and defaults are required parameters now, and the app passes AppStorageEnvironment.shared.supportDirectory and .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_conversion in TeradataTLSTests turned String(decoding:as: UTF8.self) into the failable initializer and the test went red. The client-attributes parcel body is a binary TLV buffer (big-endian u16 code, u16 length, a 0xBF charset byte), so it is not valid UTF-8 and never was; the lossy initializer had been substituting U+FFFD to make contains("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_unwrapping directives in AnalyticsHeartbeatService suppressed nothing. They were presumably right once, the code moved, and nothing said so. superfluous_disable_command is the rule that notices, and it had never run here.

swiftlint --fix broke a file, and the app build did not catch it

empty_count rewrote replaced.count > 0 into !replaced.isEmpty in MSSQLParameterBatch, where replaced is a tuple (query: String, count: Int), not a collection. TableProMSSQLCore stopped compiling.

The app build still passed, because the app scheme does not compile that target: only AllPlugins does. Anything that runs swiftlint --fix over 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_master is SQLite's own catalog table. Renaming the type would make it lie about what it queries.
  • One String(localized:) of 225 characters in AWSAuthError, 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 a func does not cover a violation inside it.

Left for its own change

RecordDecoder decodes 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 way MySQLColumnDecoding does 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.swift and TrinoWriteSQLTests.swift each held several XCTestCase subclasses and are split into one file per class, named after the class. Every test method moved verbatim.

WelcomeOutlineRows.swift in the app target: aspectRatio(contentMode: .fit) to scaledToFit(). The violation is pre-existing on main and CI's older SwiftLint does not report it, so it only shows locally. Without it swiftlint lint --strict cannot pass on a developer's machine, which is what this change is for.

Packages/TableProCore/Sources/TableProPluginKit is a symlink to Plugins/TableProPluginKit/, so including Packages also brings PluginKit under lint for the first time. Seven violations there, none of them an API change: first(...) != nil became constraints.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 a currentPluginKitVersion bump for a style fix.

Verification

  • swiftlint lint --strict: 0 violations across TablePro, Packages and PluginKit.
  • swift test --package-path Packages/TableProCore: 289 cases across 8 test targets, 0 failures.
  • App build passes. Project regenerated.
  • AllPlugins fails on this machine with oracle-nio's @TaskLocal macro expanding to unknown 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.

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.

1 participant