search

brandonbloom / fipp

Fast Idiomatic Pretty Printer for Clojure
527 stars 44 forks source link

Don't emit a nil node when an ellipsis is not required #35

Closed cichli closed 8 years ago

cichli commented 8 years ago

Nothing in the spec mentions nil nodes, and they pass through the serialize algorithm fine, but I think the correct behaviour should be to emit nothing if no ellipsis is required.

brandonbloom commented 8 years ago

When does this happen? Better yet, can you supply a test case that has a behavior change with this patch?

cichli commented 8 years ago

It happens every time pretty-coll is called, and :print-length is nil or not exceeded.

Before patch:

user> (fipp.visit/visit (fipp.edn/map->EdnPrinter {}) [])
[:group "[" [:align () nil] "]"]

After patch:

user> (fipp.visit/visit (fipp.edn/map->EdnPrinter {}) [])
[:group "[" [:align ()] "]"]

Test case added - the only reason I didn't is because all of the existing tests in fipp.edn-test were strictly testing the output of pprint, not visit, and I wasn't sure if that was intentional. I'm happy to take a stab at a PR adding more tests to ensure visit conforms to the spec in primitives.md if you'd be interested in that?

brandonbloom commented 8 years ago

Visit is not a documented/stable/public API.

Sent from my iPhone

On Jan 6, 2016, at 10:54 PM, Michael Griffiths notifications@github.com wrote:

It happens every time pretty-coll is called.

Before patch:

user> (fipp.visit/visit (fipp.edn/map->EdnPrinter {}) []) [:group "[" [:align () nil] "]"] After patch:

user> (fipp.visit/visit (fipp.edn/map->EdnPrinter {}) []) [:group "[" [:align ()] "]"] Test case added - the only reason I didn't is because all of the existing tests in fipp.edn-test were strictly testing the output of pprint, not visit, and I wasn't sure if that was intentional. I'm happy to take a stab at a PR adding more tests to ensure visit conforms to the spec in primitives.md if you'd be interested in that?

— Reply to this email directly or view it on GitHub.

brandonbloom commented 8 years ago

Sorry for the long delay on this. Decided that nil is a perfectly acceptable empty span. Updated the docs to clarify this. Thanks!

Also worth mentioning: It's not a goal of the builtin edn printer to have minimal or attractive pretty-doc structures. There are numerous normalizations that could be applied, such as eliminating empty spans or merging redundant indentation, but it doesn't seem worth the complexity.