wulkano / Aperture

Record the screen on macOS
MIT License
1.23k stars 111 forks source link

Make the Swift API use correct types #41

Closed sindresorhus closed 6 years ago

sindresorhus commented 7 years ago

And some cleanup.

sindresorhus commented 7 years ago

@LarsJK Would you be able to review?

LarsJK commented 7 years ago

One thing I would consider is to first convert the cli args into strongly typed values. Something like:

enum Action {
    case record(RecordingConfig)
    case listAudioDevices
    case usage

    init(args: [String]) throws { ... }
}

struct RecordingConfig {
   let destination: URL
   ...
}

switch try Action(arguments: CommandLine.arguments) {
    case .record(let config):
        try record(config: config)
        exit(0)
    case .listAudioDevices:
        printErr(try toJson(DeviceList.audio()))
        exit(0)
    case .usage:
        usage()
        exit(1)
    }
} catch {
    print(error.localizedDescription)
}

Action's initializer could throw an ArgumentError that gives detailed information about what is wrong with the supplied arguments..

But it looks like you are going for JSON instead of args so ¯_(ツ)_/¯

sindresorhus commented 6 years ago

One thing I would consider is to first convert the cli args into strongly typed values.

Yup. That's what I would have done if I intended this to be an actual CLI, but it's really just clue for the Node.js module. I intend on using just JSON in the future and maybe a persistent server for two-way communication.

sindresorhus commented 6 years ago

Thanks for the review @LarsJK. Many good suggestions. I really appreciate it :)