Support optional JSON output for config, list, lookup/info, outdated & search; improve tabular output#1230
Support optional JSON output for config, list, lookup/info, outdated & search; improve tabular output#1230rgoldberg wants to merge 3 commits intomas-cli:mainfrom
config, list, lookup/info, outdated & search; improve tabular output#1230Conversation
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
rgoldberg
force-pushed
the
7.0.0/190-json
branch
5 times, most recently
from
ecf7ab1 to
b8c8eb5
Compare
…outdated` & `search`. Update `README.md`. Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
rgoldberg
force-pushed
the
7.0.0/190-json
branch
from
b8c8eb5 to
210425a
Compare
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces native JSON output support for the list, outdated, search, lookup, and config commands. This is achieved through the integration of a new swift-json library and a Zsh wrapper script that uses jq and column for terminal formatting. The implementation includes a significant refactor of the CatalogApp and InstalledApp models to handle metadata key mapping and JSON serialization. Feedback highlights several performance and architectural concerns, including inefficient JSON re-parsing in the outdated command, high memory overhead from storing raw JSON strings in models, and potential performance degradation due to disk I/O during canonical path resolution. Additionally, the review points out the tight coupling created by using a magic file descriptor (FD 3) for communication between the binary and the wrapper script, and the bypass of terminal detection logic via hardcoded ANSI escape sequences.
| "(" + $version + " " * ($max_version_length - ($version | length)) + " -> " + .newVersion + ")" | ||
| ] | | ||
| join("\u001f") | ||
| ) catch ("\u001B[4;31mError:\u001B[0m Invalid data from mas: \(.)\n" | halt_error(1)) |
There was a problem hiding this comment.
Hardcoding ANSI escape sequences for error formatting in the jq script bypasses the terminal detection logic in the Swift Printer class. If the output is redirected to a file, these escape codes will be included as literal characters, which is undesirable for non-interactive use.
| outdatedApps.compactMap { installedApp, newVersion in | ||
| do { | ||
| let newVersionKey = "newVersion" | ||
| var json = try JSON.Object(parsing: .init(describing: installedApp)) |
There was a problem hiding this comment.
Parsing the JSON string back into a JSON.Object just to insert a single field is inefficient, especially when processing a large list of outdated apps. Since InstalledApp already computes and stores its JSON representation, consider exposing the JSON.Object directly or providing a method to extend it without full re-parsing.
References
- Avoid unnecessary loops, iterations, or calculations. Identify performance bottlenecks and optimize for efficiency.
| var displayPrice: String { | ||
| formattedPrice ?? "?" | ||
| private let jsonObject: JSON.Object | ||
| private let json: String |
There was a problem hiding this comment.
Storing the full JSON representation as a String in every CatalogApp instance increases memory consumption, particularly for large search results. Consider making this a computed property or generating it on demand to reduce the memory footprint of the model.
| } | ||
|
|
||
| fileprivate init(for item: NSMetadataItem) { | ||
| let valueByAttribute = item.values(forAttributes: item.attributes + [NSMetadataItemPathKey]) ?? .init() |
There was a problem hiding this comment.
Fetching all attributes via item.attributes for every item in the Spotlight query results can be very slow. It is more efficient to specify only the required attributes in the values(forAttributes:) call.
References
- Identify performance bottlenecks and optimize for efficiency.
| return (try? URL(filePath: path, directoryHint: .isDirectory).resourceValues(forKeys: [.canonicalPathKey]))? | ||
| .canonicalPath // swiftformat:disable:this indent |
There was a problem hiding this comment.
Resolving the canonicalPath involves disk I/O for every installed app found. This will significantly degrade the performance of the list command when many apps are installed. Consider if this resolution is strictly necessary for the default output.
1 participant
Support optional JSON output for
config,list,lookup/info,outdated&search; improve tabular output.Resolve #190
Resolve #412
Resolve #1091