ninxsoft / mist-cli

A Mac command-line tool that automatically downloads macOS Firmwares / Installers.
MIT License
620 stars 29 forks source link

Added options to make logging / output more machine readable #51

Closed tperfitt closed 1 year ago

tperfitt commented 2 years ago

I incorporated Mist into MDS and it has been working great! I wanted to have a more structured output for easy parsing from MDS so I added in some changes:

  1. When Mist exited with an error, it exited as termination status 0. Changed to -1.
  2. Add a new option ("-j") for mist download that outputs as messages that are structured JSON data, one per line for easy parsing. There are 3 message types: Header, Info and Progress. Header and info are just KVP with Header/Info as they key and the message as the value. The Progress message has "Progress" as the key and a json hash with the download progress (steps, data completed, etc).
  3. Added a new option for PrettyPrint for tell it if it should be structured data or not to output. This required a bunch of changes to every call that I wanted to output as structured. There may have a less intrusive way but this way works. Let me know if you have any ideas on how to make this better.
  4. renamed current -> currentBytes, total->totalBytes to make them more clear. Current was being used both as a local variable for index and for the bytes which hurt my brain.
  5. Fixed an issue where output was showing bytes remaining and completed as the end of the prior step.
  6. Message output that didn't contain data now goes to stderr. For example, all the header and info messages are stderr, but when the plist / json output for mist list is run, it goes to stdout. this makes knowing what data to use when more deterministic.
  7. The "Firmware" and "Product" objects shared many similar traits so i created a DownloadInfo struct to pass to Downloader. It probably would have been better to make this a superclass or firmware and product, but that that would have required more structural changes. Let me know if you'd rather go this route.
ninxsoft commented 2 years ago

Hi @tperfitt, thanks for the PR. There's a lot to unpack, so here goes!

  1. https://github.com/ninxsoft/Mist/blob/e5545a07489bffdf24beb55c578925d4223f67ac/Mist/Commands/Download/Download.swift#L26-L31 I don't believe this is an error - the command is running successfully and not finding a result from Apple. If there is an error retrieving results from Apple, a separate MistError is thrown.

  2. Looks good, I would just update the variable name + description from:

    https://github.com/ninxsoft/Mist/blob/e5545a07489bffdf24beb55c578925d4223f67ac/Mist/Commands/Download/DownloadOptions.swift#L47-L51

    To:

    @Flag(name: .shortAndLong, help: """
    Print output in JSON format for structured parsing.
    """)
    var jsonOutput: Bool = false
  3. To help reduce duplication of code, we can add a helper function to Dictionary+Extension.swift:

    func jsonString() -> String? {
    
        do {
            let data: Data = try JSONSerialization.data(withJSONObject: self, options: .prettyPrinted)
            return String(data: data, encoding: .utf8)
        } catch {
            Swift.print(error.localizedDescription)
            return nil
        }
    }

    This will allow us to convert any valid dictionary to a JSON string. We can then cleanup the method that prints headers from:

    https://github.com/ninxsoft/Mist/blob/e5545a07489bffdf24beb55c578925d4223f67ac/Mist/Helpers/PrettyPrint.swift#L22-L48

    To:

    static func printHeader(_ header: String, jsonOutput: Bool) {
        var string: String = ""
    
        if jsonOutput {
            let dictionary: [String: String] = ["Header": header]
    
            if let jsonString: String = dictionary.jsonString() {
                string = jsonString
            }
        } else {
            let horizontal: String = String(repeating: "─", count: header.count + 2)
            string = "┌\(horizontal)┐\n│ \(header) │\n└\(horizontal)┘"
        }
    
        Swift.print(string)
    }

    For the non-header printing method, the method signature is getting a little out of control:

    https://github.com/ninxsoft/Mist/blob/e5545a07489bffdf24beb55c578925d4223f67ac/Mist/Helpers/PrettyPrint.swift#L57

    • The first four parameters are mutually exclusive to the last 2 parameters, so I believe there is an opportunity here to split the method into two (possibly leveraging method overloading with different parameters). I don't have an answer for this just yet, just spitballing.
    • You have hardcoded structuredOutput: false in both HTTP.swift and List.swift. To promote feature parity, I would look at adding jsonOutput to ListOptions, and passing on this value to HTTP methods from either ListOptions or DownloadOptions.
    • I would also look at converting messagetype: String to a MessageType enum
  4. Looks good to me!

  5. Looks good to me, and thanks!

  6. I am not sold on the idea of repurposing stderr to filter out header + info messages. In my head, stderr should be used for errors, and errors alone. By adding structured JSON output, do we not have a way to filter out / parse stdout messages we do not care about?

  7. mist originally only supported macOS Installers, and Firmware support was added as an afterthought. Since there is a lot of overlap, it probably makes sense to create a DownloadInfo (can't think of a better name 🤔) protocol, of which both the Firmware and Product structs conform to.

    Alternatively, if it feels like too much work (and macOS Installers will eventually be phased out in favour of Firmwares...), we can leave it just the way it is 🙃.

Additionally, there are a couple of housekeeping tasks that have crept into your PR:

  1. https://github.com/ninxsoft/Mist/blob/e5545a07489bffdf24beb55c578925d4223f67ac/Mist.xcodeproj/project.pbxproj#L466

    https://github.com/ninxsoft/Mist/blob/e5545a07489bffdf24beb55c578925d4223f67ac/Mist.xcodeproj/project.pbxproj#L481

    Are you able to roll these back to 7K3HVCLV7Z?

  2. Your PR in it's current form will fail to merge as it does not pass all linting checks. SwiftLint can be installed and run manually with swiftlint --strict from the root of the project. There is also a Run SwiftLint script phase as part of the Xcode project build process:

    if which swiftlint > /dev/null ; then
     swiftlint --strict
    else
     echo "warning: SwiftLint not installed, download from https://github.com/realm/SwiftLint"
    fi
    
    exit 0

    Note: The linting rules I enforce are quite strict 😅.

  3. The same goes for docstrings - I use DrString to ensure docstrings are consistent. You can run manually with drstring check --config-file .drstring.toml from the root of the project, or via the Run DrString script phase as part of the Xcode project build process:

    if which drstring > /dev/null ; then
      drstring check --config-file "$SRCROOT/.drstring.toml" || true
    else
      echo "warning: DrString not installed, download from https://github.com/dduan/DrString"
    fi
    
    exit 0
ninxsoft commented 1 year ago

Closing due to inactivity, will re-open if required by @tperfitt 👍