Skip to content

Extract ConfigKeyKit into Examples/ConfigKeyKit Swift package#365

Open
leogdion wants to merge 8 commits into
v1.0.0-beta.2from
267-configkeykit
Open

Extract ConfigKeyKit into Examples/ConfigKeyKit Swift package#365
leogdion wants to merge 8 commits into
v1.0.0-beta.2from
267-configkeykit

Conversation

@leogdion
Copy link
Copy Markdown
Member

Splits the two divergent in-tree ConfigKeyKit copies (MistDemo and BushelCloud) into one Examples/ConfigKeyKit package with ConfigKeyKit (base) + ConfigKeyKitCLI (Command, CommandRegistry, etc.) products, both consumers now depending on it via .package(path: "../ConfigKeyKit"). BushelCloud's bushelPrefixed: conveniences move to a small ConfigKey+BUSHEL extension inside BushelCloudKit, keeping the new package application-neutral. Adds full scaffolding (CI workflow, lint pipeline, mise, LICENSE, README) and a setup-configkeykit composite action — placed inside the new package so future consumers can reference it remotely once the subrepo is published, mirroring how setup-mistkit lives in MistKit.

For #267.

Splits the two divergent in-tree ConfigKeyKit copies (MistDemo and
BushelCloud) into one Examples/ConfigKeyKit package with `ConfigKeyKit`
(base) + `ConfigKeyKitCLI` (Command, CommandRegistry, etc.) products,
both consumers now depending on it via `.package(path: "../ConfigKeyKit")`.
BushelCloud's `bushelPrefixed:` conveniences move to a small ConfigKey+BUSHEL
extension inside BushelCloudKit, keeping the new package application-neutral.
Adds full scaffolding (CI workflow, lint pipeline, mise, LICENSE, README)
and a `setup-configkeykit` composite action — placed inside the new package
so future consumers can reference it remotely once the subrepo is published,
mirroring how setup-mistkit lives in MistKit.

For #267.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 18, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7c82c4a7-0006-4d26-bbab-8e7586205d8b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 267-configkeykit

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (v1.0.0-beta.2@b0c65d8). Learn more about missing BASE report.

Additional details and impacted files
@@               Coverage Diff                @@
##             v1.0.0-beta.2     #365   +/-   ##
================================================
  Coverage                 ?   69.04%           
================================================
  Files                    ?      113           
  Lines                    ?     2630           
  Branches                 ?        0           
================================================
  Hits                     ?     1816           
  Misses                   ?      814           
  Partials                 ?        0           
Flag Coverage Δ
mistdemo-spm-macos 11.93% <ø> (?)
mistdemo-swift-6.2-jammy 12.12% <ø> (?)
mistdemo-swift-6.2-noble 11.97% <ø> (?)
mistdemo-swift-6.3-jammy 11.93% <ø> (?)
mistdemo-swift-6.3-noble 11.93% <ø> (?)
spm 67.52% <ø> (?)
swift-6.1-jammy 67.79% <ø> (?)
swift-6.1-noble 67.64% <ø> (?)
swift-6.2-jammy 67.56% <ø> (?)
swift-6.2-noble 67.60% <ø> (?)
swift-6.3-jammy 67.60% <ø> (?)
swift-6.3-noble 67.49% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member Author

@leogdion leogdion left a comment

Choose a reason for hiding this comment

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

Let's think a better way to organize ConfigKeyKit and ConfigKeyCLI. I'd prefer ConfigKeyKit contain everything and a different product contain everything but the CLI piece. Or re-evaluate whether we need to break these apart.

Comment thread .github/workflows/examples.yml Outdated
Per PR #365 review: merge the CLI target into the base ConfigKeyKit
target (one product, one test target) and remove ConfigKeyKit from the
top-level Examples matrix — the subpackage has its own ConfigKeyKit.yml
workflow.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@leogdion leogdion marked this pull request as ready for review May 18, 2026 16:32
Content moves to brightdigit/ConfigKeyKit; the next commit re-adds it
at the same path via git-subrepo.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented May 18, 2026

Code Review — PR #365: Extract ConfigKeyKit into Examples/ConfigKeyKit Swift Package

Overview

The structural goal is correct and well-executed. Extracting the duplicated ConfigKeyKit code into a standalone package is the right move, and both consumers (MistDemo, BushelCloud) are correctly updated to depend on it via local path. The concurrency model (CommandRegistry as an actor) is correct, all types are Sendable, and the test suite uses Swift Testing properly (@Test, #expect, @Suite, parameterised tests). The CI/workflow scaffolding closely mirrors the established patterns.


Medium Priority

1. Stale read() method references in doc comments
ConfigKey.swift (lines 33, 42) and OptionalConfigKey.swift (lines 33, 39) reference a read() method that does not exist anywhere in the package. These are stale from an earlier design and will confuse consumers reading generated documentation. Reword or remove.

2. Unnecessary internal import Foundation in CommandLineParser.swift (line 30)
The implementation only uses CommandLine (stdlib), String.hasPrefix (stdlib), and Array.dropFirst (stdlib). Foundation is unused here. This matters for WASI targets and contradicts the package's "dependency-free" positioning in the README.

3. public import Foundation in CommandRegistryError.swift (line 30)
Should be internal import Foundation. Using public re-exports Foundation transitively to all consumers of ConfigKeyKit, which is unnecessarily broad.

4. Duplicated key(for:) logic between ConfigKey and OptionalConfigKey
Both structs implement identical logic (check explicitKeys, fall back to baseKey + style lookup). If this body diverges between the two structs in a future change, it will introduce silent bugs. Consider a shared private helper or a protocol default implementation with baseKey, styles, and explicitKeys as requirements.

5. ConfigKey+Bool.swift repeats the full init body instead of delegating
The ConfigKey<Bool> specialisation copies the entire init(cli:env:default:) body. If base init logic ever changes, the Bool override won't track it. Delegate instead:

extension ConfigKey where Value == Bool {
  public init(cli: String, env: String, default defaultVal: Bool = false) {
    self.init(cli: cli, env: env, default: defaultVal)
  }
}

Low Priority / CLAUDE.md Compliance

6. Unnecessary inline comments (CLAUDE.md: "Default to writing no comments. Only add one when the WHY is non-obvious.")

  • ConfigKey.swift lines 98, 103: // Check for explicit key first, // Generate from base key and style
  • OptionalConfigKey.swift lines 87, 92: same
  • CommandLineParser.swift lines 43, 49, 64, 69: // Skip the executable name, // If it starts with '--', etc.

All of these describe what the code plainly does — remove them.

7. Consider stdlib-only string replacement in StandardNamingStyle
replacingOccurrences(of:with:) requires Foundation. A pure-stdlib alternative like base.uppercased().split(separator: ".").joined(separator: "_") would make StandardNamingStyle.swift Foundation-free, removing the last Foundation dependency outside of error types.

8. Unnecessary import Foundation in test file
CommandRegistryTests+Errors.swift line 30 imports Foundation but only uses CommandRegistryError and Testing. Remove it.

9. PR description mentions a two-product split (ConfigKeyKit + ConfigKeyKitCLI)
The implementation uses a single consolidated ConfigKeyKit product. The description should be updated to avoid reviewer confusion.


Test Coverage

10. No test for the ConfigurationParseable convenience extension (BaseConfig == Never path)
TestCommandTypes.swift already provides TestConfig(configuration:base:), so adding a single test calling TestConfig(configuration: TestConfigReader()) would close this gap.

11. mixedConcurrentOperations test is structurally correct (actor serialisation guarantees the ordering), but the absence of comments makes the test's intent non-obvious to a reader. A one-line explanation of why the post-group assertion is safe would help.


CI / Workflow

No concerns. The matrix strategy, concurrency cancellation, Codecov upload, and lint gate are correctly structured and mirror MistKit's patterns. The setup-configkeykit composite action's bash/PowerShell sed interpolation is well-formed.


Security

No concerns. The package only resolves key strings from environment variables and CLI arguments — it does not execute, store, or log values.


Summary Table

Priority Location Issue
Medium ConfigKey.swift:33,42, OptionalConfigKey.swift:33,39 Remove stale read() references from doc comments
Medium CommandLineParser.swift:30 Remove unnecessary internal import Foundation
Medium CommandRegistryError.swift:30 Change public import Foundationinternal import Foundation
Medium ConfigKey.swift / OptionalConfigKey.swift Duplicated key(for:) logic — share via helper or protocol extension
Medium ConfigKey+Bool.swift Delegate to base init instead of copying body
Low ConfigKey.swift:98,103, OptionalConfigKey.swift:87,92, CommandLineParser.swift:43,49,64,69 Remove what-the-code-does inline comments (CLAUDE.md)
Low StandardNamingStyle.swift Drop Foundation dep with stdlib-only string split/join
Low CommandRegistryTests+Errors.swift:30 Remove unused import Foundation in test
Low PR description Correct two-product mention — implementation is single product
Low Tests Add test for ConfigurationParseable BaseConfig == Never convenience path

Overall this is a clean, well-structured extraction and a clear improvement over the duplicated state. The issues above are mostly minor quality and documentation polish rather than correctness bugs.

🤖 Review generated with Claude Code

…s/ConfigKeyKit

subrepo:
  subdir:   "Examples/ConfigKeyKit"
  merged:   "8254da6"
upstream:
  origin:   "git@github.com:brightdigit/ConfigKeyKit.git"
  branch:   "main"
  commit:   "8254da6"
git-subrepo:
  version:  "0.4.9"
  origin:   "https://github.com/Homebrew/brew"
  commit:   "6f293daa9f"
@claude
Copy link
Copy Markdown

claude Bot commented May 18, 2026

Code Review — PR #365: Extract ConfigKeyKit into Examples/ConfigKeyKit

Overview

This PR eliminates duplicated ConfigKeyKit code that existed separately in Examples/MistDemo and Examples/BushelCloud by extracting it into a standalone Examples/ConfigKeyKit subrepo package. BushelCloud-specific helpers (bushelPrefixed:) move to BushelCloudKit/Configuration/ConfigKey+BUSHEL.swift. Both consumers now depend on the shared package via path: "../ConfigKeyKit".

This is a good structural refactor — the duplication was a maintenance risk and the canonical MistDemo version was the more complete one.


What's working well

  • Deduplication is clean: The right copy (MistDemo) was used as the canonical source. BushelCloud's simpler/older copy is discarded with no behavioral regression.
  • Sendable conformance additions: CommandConfiguration and CommandLineParser now explicitly conform to Sendable — correct Swift 6 hygiene that was missing.
  • Removed spurious import Foundation: Several source files were importing Foundation without needing it. Cleaning this up reduces unnecessary dependencies.
  • Tests are comprehensive: The new package carries all the MistDemo-origin tests plus new ConfigKeyTests / ConfigKeySourceTests, replacing the thinner BushelCloud set that's deleted.
  • CI workflow: Full matrix covering Ubuntu (noble + jammy), macOS, Windows, Android, and WASM — consistent with MistKit's own pipeline.
  • setup-configkeykit composite action: Good pattern for future subrepo publishing, mirrors setup-mistkit.

Issues to address

1. PR description contradicts Package.swift — minor but confusing

The description says: "Splits … into one Examples/ConfigKeyKit package with ConfigKeyKit (base) + ConfigKeyKitCLI (Command, CommandRegistry, etc.) products"

But Package.swift exposes a single ConfigKeyKit product that bundles both key types and CLI scaffolding:

products: [
  .library(name: "ConfigKeyKit", targets: ["ConfigKeyKit"]),
],

The README reflects reality ("A single ConfigKeyKit product bundles: Key types + CLI scaffolding") but the PR description will confuse reviewers. Please update the description to match — or, if the split-product design is still intended, it should be implemented here, not left as implied.

2. Shell injection risk in setup-configkeykit action

run: |
  sed -i '' 's|...|.package(url: "...", branch: "'"${{ inputs.branch }}"'")|g' Package.swift

${{ inputs.branch }} is expanded at workflow parse time (before shell execution), so a branch name containing |, ', or \ could break the sed command or produce unexpected Package.swift output. Since this is an internal developer action the practical risk is low, but it's worth quoting the substitution more defensively or using a dedicated script with proper escaping.

3. internal property visibility in ConfigKey.swift

In the new canonical ConfigKey.swift, styles and explicitKeys changed from private to internal:

// New
internal let styles: [ConfigKeySource: any NamingStyle]
internal let explicitKeys: [ConfigKeySource: String]

These are implementation details. private is sufficient — nothing outside the struct should need direct access. If the debug extensions need them, they're in the same file. Please verify this is intentional and not an accidental visibility promotion.

4. Merge-order conflict with PR #363

PR #363 (currently open) modifies Examples/BushelCloud/Sources/ConfigKeyKit/ConfigKey.swift and its siblings (adding internal import modifiers). This PR deletes those same files. Whichever lands second will need a rebase. Suggest landing this PR (#365) first to eliminate the files, then rebasing #363 on top.

5. CLAUDE.md not updated

CLAUDE.md references Examples/MistDemo/Sources/ConfigKeyKit/ in several places implicitly (the MistDemo architecture section). Now that ConfigKeyKit is a separate package, it may be worth adding a brief mention of Examples/ConfigKeyKit to the package structure diagram and the MistDemo section. Not blocking, but keeps docs in sync.


Security

No concerns. The package is configuration-key metadata (string key names + defaults) with no network access, no credential storage, and no dynamic code.

Test coverage

Good. New ConfigKeySourceTests, expanded ConfigKeyTests, and the full CommandRegistry suite are all migrated. The one gap is that bushelPrefixed: convenience initializers in ConfigKey+BUSHEL.swift (BushelCloudKit) have no dedicated test — the only coverage is the deleted BushelCloud tests that used them. Consider adding a test in BushelCloudKitTests for the new ConfigKey+BUSHEL.swift extension.


Overall: The refactor is sound and well-executed. Address the internal vs private property visibility question and the PR description accuracy before merging.

🤖 Reviewed with Claude Code

leogdion and others added 4 commits May 18, 2026 18:25
- Switch macOS scheme from `<PACKAGE>-Package` to just `ConfigKeyKit`
  (Package.swift uses swift-tools-version 6.2, no auto `-Package`
  scheme).
- Drop Swift 6.1 from Ubuntu/Windows/Android matrices and remove the
  now-redundant 6.1+wasm excludes.
- Drop macos-15 + Xcode 16.3/16.4 entries from build-macos-platforms;
  those Xcode releases ship Swift 6.1 and can't open a 6.2 manifest.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
subrepo:
  subdir:   "Examples/ConfigKeyKit"
  merged:   "2ec9b84"
upstream:
  origin:   "git@github.com:brightdigit/ConfigKeyKit.git"
  branch:   "main"
  commit:   "2ec9b84"
git-subrepo:
  version:  "0.4.9"
  origin:   "https://github.com/Homebrew/brew"
  commit:   "6f293daa9f"
Swift Testing's parallel runner dies mid-flight on Windows 6.2 when
running the async/actor tests in CommandRegistryTests — the process
exits with code 1 after sync tests pass but before most async tests
ever finish. Sync tests pass; non-CommandRegistry async tests pass;
same suite passes on Windows 6.3, Linux 6.2, and macOS 6.2.

Gate the whole CommandRegistry suite with a `.disabled(if:)` trait
driven by a `TestEnvironment.isWindowsSwift6_2OrEarlier` static, so
Windows 6.3 stays in the matrix without the noise.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
subrepo:
  subdir:   "Examples/ConfigKeyKit"
  merged:   "cd3d609"
upstream:
  origin:   "git@github.com:brightdigit/ConfigKeyKit.git"
  branch:   "main"
  commit:   "cd3d609"
git-subrepo:
  version:  "0.4.9"
  origin:   "https://github.com/Homebrew/brew"
  commit:   "6f293daa9f"
@claude
Copy link
Copy Markdown

claude Bot commented May 18, 2026

Code Review: Extract ConfigKeyKit into Examples/ConfigKeyKit Swift package

Overview

This PR consolidates two divergent in-tree copies of ConfigKeyKit (one in MistDemo, one in BushelCloud) into a single canonical package at Examples/ConfigKeyKit/, tracked as a git-subrepo. Both consumers switch to path: "../ConfigKeyKit" dependencies. BUSHEL-specific convenience initializers are cleanly separated into a new ConfigKey+BUSHEL.swift extension inside BushelCloudKit, keeping the shared package application-neutral. The motivation is sound and the execution is largely clean.


Issues

Bug / Test Gap

Missing tests for ConfigKey+BUSHEL.swift
BushelCloud's deleted Tests/ConfigKeyKitTests/ConfigKeyTests.swift and OptionalConfigKeyTests.swift contained tests for bushelPrefixed initializers that no longer have a home. The new ConfigKey+BUSHEL.swift in BushelCloudKit introduces three public init(bushelPrefixed:) overloads with no test coverage. A BushelCloudKit test target (or dedicated suite) should verify these inits produce the correct CLI/ENV key strings.


Lint / Script

lint.sh — double --configuration flag (inherits from root lint.sh)

SWIFTFORMAT_OPTIONS is always --configuration .swift-format, and the lint command also hardcodes --configuration .swift-format, so the flag is passed twice. swift-format silently uses the last occurrence, but it is confusing and fragile. Either remove the hardcoded flag from the lint line or stop setting SWIFTFORMAT_OPTIONS to include it. (The same issue exists in the root Scripts/lint.sh, so this is a copy of an existing bug — worth fixing both in a follow-up.)

lint.sh — unquoted $PACKAGE_DIR (also inherits from root)

pushd $PACKAGE_DIR should be pushd "$PACKAGE_DIR" to guard against paths with spaces.


CI / Actions

setup-configkeykit — sed injection risk

The inputs.branch value is expanded at workflow-parse time and injected directly into the sed replacement string. A branch name containing |, \, or a newline would corrupt the command. Consider writing the branch to an env var first and using a safer substitution approach, or add an if guard that validates the branch name is alphanumeric/dash/slash before running sed.


Design / Architecture

PR description mentions a ConfigKeyKitCLI product that does not exist
The description says: "one ConfigKeyKit (base) + ConfigKeyKitCLI (Command, CommandRegistry, etc.) products". The actual Package.swift ships a single ConfigKeyKit product that bundles both key types and CLI scaffolding. The README is accurate; the description is stale. Worth updating the description or opening a follow-up to actually separate the products.

Bundling CLI scaffolding (Command, CommandRegistry, CommandLineParser) in the base product means consumers that only need key-resolution also pull in command-dispatch infrastructure. Not a blocker, but worth tracking as a future split.

Platform minimums may be overly restrictive
The new Package.swift requires macOS 15 / iOS 18 / watchOS 11 / visionOS 2. ConfigKeyKit has no platform-specific APIs and zero platform dependencies (Foundation was intentionally removed in this PR). Dropping the floor to macOS 13 / iOS 16 would broaden adoption without any implementation cost.


Positives

  • Removing all 20 import Foundation statements is exactly right — the package is now truly dependency-free as advertised.
  • Moving BUSHEL-specific inits to BushelCloudKit keeps the shared package application-neutral without breaking the BushelCloud surface area.
  • Deleting the deprecated boolDefault accessor from the BushelCloud version is clean.
  • Full CI/lint/mise/periphery scaffolding mirrors the MistKit pattern consistently.
  • The setup-configkeykit composite action is a nice pattern for testing consumers against unreleased branches — mirrors setup-mistkit well.
  • The ConfigKeyKitTests in the shared package provide solid coverage of the core types.

Summary

The extraction is architecturally correct and the shared package is cleaner than either predecessor. The test-coverage gap for ConfigKey+BUSHEL.swift is the one item that should be addressed before merge. The lint-script and sed-injection issues are low severity but worth a follow-up for consistency.

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