brandonbloom / fipp

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

Add :print-length and :print-level options #30

Closed cichli closed 8 years ago

cichli commented 8 years ago

Hi, first of all, thanks for the great (and fast!) library :-).

Both options are analogous to (and default to) *print-length* and *print-level* respectively.

I also took a stab at improving the benchmark, using test.check to generate sample data using any-printable. Additionally, I've switched to using a stubbed Writer implementation instead of with-out-str to try and minimise any overhead.

Here are results for the new benchmark, both before and after this change is applied - the impact is reasonably negligible for normal usage (i.e. when :print-length / :print-level are not set). Please do try and verify these results locally!

Notes on the implementation:

Motivation for this change: we now use fipp as the default pretty-printer in CIDER, but would like a way to prevent accidental printing of infinitely long or recursive structures :-). Let me know if there's anything I've missed or you think I should change.

brandonbloom commented 8 years ago

Hi Michael,

It's very exciting to hear that Fipp is now the default in Cider! Thanks for taking on this work.

I might not have a chance for a few days to pull down the code and play with it. I'll take a super quick look at the code now and make some comments. While it's OK by me if the engine code gets a little hairy, I'd like to do my best to keep the pretty-document printer code as clean as possible, as it's a good reference for people writing printers for non-edn.

Cheers, Brandon

brandonbloom commented 8 years ago

Looking at this code more carefully now... additional comments follow.

brandonbloom commented 8 years ago

Removed the benchmarking commit, squashed the remaining commits, cleaned up the code a bit, fixed some edge cases, updated the docs.

@cichli Please try the master branch and make sure it still works as you'd expect. If it does, I'll cut a release.

cichli commented 8 years ago

Hi Brandon,

Thanks a lot for taking the time to look at the PR - and then to subsequently clean it up and merge it! pretty-coll is indeed a much cleaner impl. I've checked out the changes locally and they work exactly as I'd expect. Looking forward to the release!

I'll add my part to the benchmarking discussion on the other PR.

Thanks,

Michael

brandonbloom commented 8 years ago

OK released version 0.6.3 - Thanks again!