Uh oh!
There was an error while loading. Please reload this page.
♻️ Modernize concurrency, tests and service conventions + Claude Code setup - #47
Conversation
Cover previously untested areas: multi-language transforms, configuration decoding/migration, file I/O with plist prefix routing, ValueRange edge cases, GoogleError detection, and LocRow formatting edge cases (91 tests total). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract filesystem operations behind an internal FileSystem protocol so SaveMappedValuesTests can use an in-memory mock instead of writing to temp directories on disk. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move Combine API out of protocol definitions and core class into a dedicated CombineExtensions.swift that forwards to async/sync implementations. SheetsAPIServicing now defines only async methods, run() is fully async, and URLSessionExtensions.swift is removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Sources/ACKLocalizationCore/ACKLocalization.swift:38
run()only handlesLocalizationErrorexplicitly;RequestError(thrown bySheetsAPIService) and other errors fall into the generic catch and get printed to stdout without the existing formatting / missing-sheet-tab mapping. This is a behavior regression compared to the previous pipeline where request failures were surfaced asLocalizationError.
} catch let error as LocalizationError {
displayError(error)
exit(1)
} catch {
print(error)
Tests/ACKLocalizationCoreTests/Mocks/FileSystemMock.swift:5
createdDirectoriesis an array, but the doc comment says it is "keyed by path" (which would imply a dictionary). This is misleading for future test authors.
Sources/ACKLocalizationCore/Services/SheetsAPIServiceImpl.swift:38- Force-unwrapping the percent-encoded sheet name and the URL (
addingPercentEncoding(...)!,urlComponents.url!) can crash the process on malformed input. Since this is user-supplied data (sheet/tab names), prefer failing withRequestErrorinstead of crashing.
let sheetName = sheetName ?? spreadsheet.sheets.first?.properties.title ?? ""
var urlComponents = URLComponents(string: "https://sheets.googleapis.com/v4/spreadsheets/" + spreadsheet.spreadsheetId + "/values/" + sheetName.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed)!)!
urlComponents.queryItems = [URLQueryItem(name: "valueRenderOption", value: "UNFORMATTED_VALUE")]
var request = URLRequest(url: urlComponents.url!)
credentials?.addToRequest(&request)
Uh oh!
There was an error while loading. Please reload this page.
@olejnjak Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
IgorRosocha
left a comment
There was a problem hiding this comment.
LGTM, thanks for taking your time @olejnjak! 🙌
Uh oh!
There was an error while loading. Please reload this page.
Changes
Concurrency & tests
FileSystemprotocol to decouple file I/O from testsService conventions
RequestErrorfromSheetsAPIServiceusing typed throwsImpl)Claude Code setup
CLAUDE.mdproject guidance and code style / service convention rules.claude/settings.json(permissions, sandbox rules, plugins) and.mcp.jsonwith Xcode MCP serverCloses#43
Closes#19