Skip to content

refactor(plugins): unify the package PluginKit on the framework contract (R-001)#1500

Merged
datlechin merged 2 commits into
mainfrom
refactor/pluginkit-single-source
May 29, 2026
Merged

refactor(plugins): unify the package PluginKit on the framework contract (R-001)#1500
datlechin merged 2 commits into
mainfrom
refactor/pluginkit-single-source

Conversation

@datlechin
Copy link
Copy Markdown
Member

@datlechin datlechin commented May 29, 2026

Summary

Phase 1 (R-001): the SPM TableProPluginKit (Packages/TableProCore/Sources/TableProPluginKit) was a stale, divergent copy of the Xcode-framework TableProPluginKit (Plugins/TableProPluginKit). This promotes the package to the framework's (current, ABI v16) content so the two trees are byte-identical except Info.plist. The physical collapse to one source is the follow-up (7.2).

How to review

Don't read the 2000-line diff line by line. Instead:

  1. Run diff -qr Plugins/TableProPluginKit Packages/TableProCore/Sources/TableProPluginKit — it should report only Info.plist. That proves "package == framework now."
  2. Review only the three real logic changes below.

The three logic changes

  • #if canImport(AppKit) gates DocumentInspectorPlugin in both trees (the only macOS-only file; InspectorWindowFactory uses NSDocument/NSWindowController). macOS compiles it unchanged; iOS excludes the block. The protocol itself (documentClass: AnyClass) is platform-neutral.
  • QueryResult.init(from: PluginQueryResult) converts PluginCellValue -> String? (null→nil, text→value, bytes→uppercase hex). This is the entire downstream cascade of PluginQueryResult.rows becoming [[PluginCellValue]]. Mobile never constructs PluginQueryResult nor calls this init, and QueryResult.rows/Cell are unchanged, so there is no mobile data-path migration. Covered by a new test (mapsCellValuesToLegacyStrings).
  • Two package-only files removed:
    • CompletionEntry.swiftnot abandoned; relocated. The framework defines CompletionEntry inside SQLDialectDescriptor.swift, so once that file is adopted, the standalone copy would be a duplicate definition. CompletionEntry is still used by the desktop autocomplete.
    • PluginPagedResult.swiftdead (zero references repo-wide; tied to the dropped legacy paging API).

Drift baseline trimmed (.github/duplicate-contract-baseline.txt): all pluginkit: entries removed except Info.plist.

Risk Addressed

  • R-001: duplicate, drifting PluginKit contracts (a plugin could compile against one and load against another).

Verification

  • swift build / swift test --package-path Packages/TableProCore (host; 62 tests).
  • swift build --triple arm64-apple-ios17.0 --sdk <iphoneos> — package cross-compiles for iOS (proves AppKit gating + iOS-safety).
  • CI Run iOS Tests pass — full mobile build/tests green.
  • xcodebuild -scheme TablePro build (macOS app).
  • scripts/audit-refactor-health.sh --check (drift gate green).

Notes

  • ABI bump required: no. The framework (what plugins link) is unchanged except the macOS-no-op #if canImport(AppKit) gate; currentPluginKitVersion stays 16. No plugin rebuild.
  • Forward note: the bytes → hex conversion in QueryResult.init(from:) is currently callerless (kept deterministic, pinned by test). Whoever wires the desktop path (8.2b) through this init should revisit it — the desktop wants full PluginCellValue fidelity, not hex String?.
  • Unblocks 7.2 (collapse the two trees into one physical source) and 8.2b (desktop import TableProModels once there is a single TableProPluginKit module).

@datlechin datlechin merged commit 345692e into main May 29, 2026
6 checks passed
@datlechin datlechin deleted the refactor/pluginkit-single-source branch May 29, 2026 21:46
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cc8ec90751

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +493 to +499
var hex = "X'"
hex.reserveCapacity(2 + data.count * 2 + 1)
for byte in data {
hex.append(String(format: "%02X", byte))
}
hex.append("'")
return hex
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Make byte parameter literals dialect-specific

When a driver relies on this default executeParameterized implementation, .bytes parameters are substituted as X'...'; I checked the plugin tree and Oracle/BigQuery do not provide their own executeParameterized overrides, while both can surface binary values as PluginCellValue.bytes. Row edits or user-parameterized queries involving RAW/BYTES columns on those drivers will therefore emit a literal form those dialects do not accept, so the save/query fails despite the new binary cell support. Please either require those drivers to override byte binding or move byte literal generation behind a dialect-specific hook.

Useful? React with 👍 / 👎.

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