Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

So many changes #4

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

So many changes #4

wants to merge 14 commits into from

Conversation

jgdavey
Copy link

@jgdavey jgdavey commented Mar 27, 2017

There's a fair bit here, and each commit is fairly self-explanatory, but just about every commit built on the last.

The high-level stuff:

  • Protocol-based rendering at the top-level
  • Tabular renderers have a more explicit API (record instead of ns)
  • Adds escaping for various formats to prevent breaking when printed, etc
  • Adds JSON renderer

@jgdavey
Copy link
Author

jgdavey commented Mar 30, 2017

Also, it was nice to meet you the other day.

@joegallo
Copy link
Owner

Sweet, I love the unicode renderer. That's awesome.

Sorry I've been AFK on this for the last while -- I'll get my head above water with work stuff soon and then I'll be able to look at this closer.

I really appreciate you sending this my way, and it will get in.

@pjstadig
Copy link

pjstadig commented Oct 6, 2017

I AM ONE DEGREE OF SEPARATION BETWEEN YOU TWO

@j0ni
Copy link

j0ni commented Mar 1, 2018

@joegallo bump - would be great to get this merged!

@timothypratley
Copy link

bump :) @joegallo hi

@onetom
Copy link

onetom commented Jul 1, 2021

I like most of the ideas here. The unicode table is very nice!
Having a JSON dependency baked in for such a small utility library though, is not a great idea.
I'm also unclear on the benefit of a JSON "table format", as opposed to directly calling a JSON encoder.
There is no example of the JSON support in the README, so I might misunderstand what it does.

The main problem with depending on Cheshire specifically, is that it has quite a problematic dependency tree:

$ clojure -Stree -Sdeps '{:deps {cheshire/cheshire {:mvn/version "5.10.0"}}}'
org.clojure/clojure 1.10.3
  . org.clojure/spec.alpha 0.2.194
  . org.clojure/core.specs.alpha 0.2.56
cheshire/cheshire 5.10.0
  . com.fasterxml.jackson.core/jackson-core 2.10.2
  . com.fasterxml.jackson.dataformat/jackson-dataformat-smile 2.10.2
    . com.fasterxml.jackson.core/jackson-core 2.10.2
  . com.fasterxml.jackson.dataformat/jackson-dataformat-cbor 2.10.2
    . com.fasterxml.jackson.core/jackson-core 2.10.2
  . tigris/tigris 0.1.2

These jackson libs are used in many other libraries and their versions are not very well maintained, which often lead to dependency conflicts in bigger projects:

Recently I've lost about a day because of this, before I finally found a workaround for my specific situation:
metosin/muuntaja#121

The performance of clojure.data.json has improved a lot lately and it also has pprint functionality, so in many cases, it's reasonable to trade off jackson's performance for clojure.data.json's zero-dependency nature.

I think it's important to leave the choice of the JSON codec to the consumers of this library somehow or simply don't provide JSON specific features in this core lib.

For more background, here is a recent article, which compares the various Clojure JSON libs: https://www.juxt.pro/blog/json-in-clojure

@escherize
Copy link

Looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants