Improved Ink CLI tool #19

john-mueller · 2019-12-01T05:43:10Z

Added -h/--help and --version tags
Changed functionality of calling ink to make it more testable and permit standard forms of input (pipes, redirections, etc.)

$ ink reads from stdin
$ ink file.md reads contents of file
$ ink -m string parses string directly

Added -h/--help and --version tags Changed functionality of calling ink to make it more testable and permit standard forms of input (pipes, redirections, etc. `$ ink` reads from stdin `$ ink file.md` reads contents of file `$ ink -m string` parses string directly

john-mueller · 2019-12-01T05:55:38Z

This is in preparation for PR #20 discussing CommonMark testing and benchmarking tools. These changes are necessary in order to support testing and benchmarking using the CommonMark tools (specifically, being able take a filename and parse its contents, read from stdin, and redirection). The upshot is a much better CLI tool, without too much additional complexity, so I separated it into its own PR.

The readme and the tool's usage message now provide more examples and documentation. There's probably also pedagogical value in showing how to make a tool that accepts differing forms of command line input.

This is actually pretty extensively tested on this branch, but because the tests work by directly calling the build product through bash, they require a symlink to be created before they will pass. In my mind, this makes the tests unsuitable for general inclusion here. Suffice it to say, I'm confident it will work as described.

JohnSundell

Thanks for this @john-mueller, I'm really excited to get the infra needed for CommonMark benchmarking in place 👍 Left some comments inline, let me know what you think. I'd also love to hear what @ezfe and @bamx23 thinks about this slight redesign of the CLI.

JohnSundell · 2019-12-01T22:15:50Z

Sources/InkCLI/Printing.swift

+
+import Foundation
+
+internal var versionMessage: String = "Ink (v0.1.2): Markdown -> HTML converter"


Unless we automate this, there's a very little chance that it'll stay up to date, since it'd require manually modifying the source code each time I make a new release. I suggest dropping it, at least for now.

Yeah, I thought of that. Not a big deal to omit.

JohnSundell · 2019-12-01T22:19:20Z

Sources/InkCLI/Printing.swift

+
+internal enum Output { case standardOut, standardError }
+
+internal func print(_ message: String, on output: Output) {


Instead of introducing a new print function, why not call fputs directly from main.swift? And if you wish to wrap it, how about creating a dedicated printError(_ error:) function instead, since Swift.print doesn't need wrapping?

Changed so only fputs is wrapped. I like the printError wrapper, as fputs doesn't append a newline like print, so having parallel calls makes it easier to avoid mistakes.

JohnSundell · 2019-12-01T22:22:12Z

Sources/InkCLI/main.swift

+let markdown: String

-if markdown == "-" {
+if CommandLine.arguments.count == 1 {


I think this could be implemented in a very clean way using pattern matching:

let arguments = CommandLine.arguments switch arguments.count { case 1: // parse stdin case 2: // parse file case 3 where arguments[1] == "-m" || arguments[1] == "--markdown": // parse inline markdown default: // error handling }

Much cleaner. I left the -m flag check as the second case, so that if you run ink -m or ink -m string1 string2 you get an error message specific to the -m flag.

Yeah the new implementation looks fantastic 👍

JohnSundell · 2019-12-01T22:24:07Z

Sources/InkCLI/main.swift

+    do {
+        // this is 5x more efficient than 'let data = try String(contentsOf: fileUrl)'
+        let data = try Data(contentsOf: fileUrl)
+        guard let string = String(data: data, encoding: .utf8) else {


This can be implemented as String(decoding: data, as: UTF8.self) to avoid an optional.

JohnSundell · 2019-12-01T22:24:45Z

Sources/InkCLI/main.swift

 let parser = MarkdownParser()
-print(parser.html(from: markdown))
+print(parser.html(from: markdown), on: .standardOut)
+exit(0)


exit(0) is implicit at the end of a program, no need to specify it.

bamx23

Great work! That's what I was initially thinking about.

bamx23 · 2019-12-02T06:36:44Z

Sources/InkCLI/main.swift

+case 2:
+    // single argument, parse contents of file
+    let fileUrl: URL
+    if CommandLine.arguments[1].hasPrefix("/") {


Maybe we should also check for ~ and replace it with a home directory path?

Great idea! There's an NSString API for that:

NSString(string: CommandLine.arguments[1]).expandingTildeInPath

And since we depend on Foundation for the CLI, it's no problem doing that bridging IMO.

Good idea. Bash was expanding ~ to an absolute string, so ink ~/file.md would parse correctly through the CLI's absolute path, but ink "~/file.md" would look for /$PWD/~/file.md. Doesn't hurt to add a case for this.

Really great attention to detail 👍

bamx23 · 2019-12-02T06:38:00Z

Sources/InkCLI/main.swift

+    } catch {
+        printError(error.localizedDescription)
+        printError(usageMessage)
+        exit(2)


What do you think about making an enum with exit codes?

After reading this, I think we should either use exit(1) for all errors with no enum, or start error numbering from 3 with an enum. What do you think about the case names?

I think exit(1) is just fine. Or is it planned to use these exit codes somewhere?

I think exit(1) would work fine, as it's not really used anywhere, and it reduces code. I originally had numbered them sequentially before really reading anything about standards for error codes. Especially since the user already gets a error message describing the issue, multiple exit codes seems redundant.

bamx23 · 2019-12-02T17:24:14Z

Sources/InkCLI/ExitCode.swift

+enum ExitCode: Int32 {
+    case normal = 0
+    case badMarkdownFlagUsage = 3
+    case problemReadingFile


Maybe it would be better to explicitly write every code number here?

JohnSundell

Awesome! Thanks so much for this @john-mueller, merging this in, and will take a look at your second PR as well either today or tomorrow 👍 Really appreciate your work on this.

Improved Ink CLI tool

a715dd4

Added -h/--help and --version tags Changed functionality of calling ink to make it more testable and permit standard forms of input (pipes, redirections, etc. `$ ink` reads from stdin `$ ink file.md` reads contents of file `$ ink -m string` parses string directly

john-mueller mentioned this pull request Dec 1, 2019

CommonMark testing #20

Open

JohnSundell reviewed Dec 1, 2019

View reviewed changes

Code style fixes for PR

ad6a47c

bamx23 reviewed Dec 2, 2019

View reviewed changes

Handle filepaths with ~ and add ExitCode enum

21934bd

bamx23 reviewed Dec 2, 2019

View reviewed changes

Removed ExitCode enum

e2f1960

JohnSundell approved these changes Dec 2, 2019

View reviewed changes

JohnSundell merged commit 4611014 into JohnSundell:master Dec 2, 2019

john-mueller deleted the cli-improvements branch December 2, 2019 22:42


		import Foundation

		internal var versionMessage: String = "Ink (v0.1.2): Markdown -> HTML converter"


		internal enum Output { case standardOut, standardError }

		internal func print(_ message: String, on output: Output) {

Improved Ink CLI tool #19

Improved Ink CLI tool #19

Uh oh!

Conversation

john-mueller commented Dec 1, 2019

Uh oh!

john-mueller commented Dec 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohnSundell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bamx23 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-mueller Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnSundell left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

john-mueller commented Dec 1, 2019 •

edited

Loading

john-mueller Dec 2, 2019 •

edited

Loading