clojure-emacs / parseedn

EDN parser for Emacs Lisp
58 stars 14 forks source link

Fix hash map printing #1

Closed ak-coram closed 4 years ago

ak-coram commented 4 years ago

Hi,

the printing of hash-maps only seems to work for maps containing a single (or zero) entries:

(parseedn-print-str 
 (let ((ht (make-hash-table)))
   (puthash :a 1 ht)
   (puthash :b 2 ht)
   (puthash :c 3 ht)
   ht))
Debugger entered--Lisp error: (wrong-type-argument listp :b)
  car(:b)
  mapcar(car (:b :c))
  a-keys((:b :c))
  (let ((keys (a-keys map))) (parseedn-print (car keys)) (insert " ") (parseedn-print (a-get map (car keys))) (let ((next (cdr keys))) (if (not (seq-empty-p next)) (progn (insert ", ") (parseedn-print-kvs next)))))
  parseedn-print-kvs((:b :c))
  (progn (insert ", ") (parseedn-print-kvs next))
  (if (not (seq-empty-p next)) (progn (insert ", ") (parseedn-print-kvs next)))
  (let ((next (cdr keys))) (if (not (seq-empty-p next)) (progn (insert ", ") (parseedn-print-kvs next))))
  (let ((keys (a-keys map))) (parseedn-print (car keys)) (insert " ") (parseedn-print (a-get map (car keys))) (let ((next (cdr keys))) (if (not (seq-empty-p next)) (progn (insert ", ") (parseedn-print-kvs next)))))
  parseedn-print-kvs(#<hash-table eql 3/65 0x2d1d8ad>)

I've added a fix and a test for this, please let me know if I've missed anything. Thanks!

plexus commented 4 years ago

Looks good I think, but seems like our build is broken. I'll try to fix it here so you can rebase and get a green build: https://github.com/clojure-emacs/parseedn/pull/2

plexus commented 4 years ago

That seems to have worked, could you rebase on master?

ak-coram commented 4 years ago

That seems to have worked, could you rebase on master?

Hi, thank you. I've rebased my changes on the new master.

ak-coram commented 4 years ago

Looks like my test is broken, I don't think we can rely on a specific serialization to test against since the keys are not in a guaranteed order (I think we're relying on maphash to retrieve the keys). Maybe read back the result and compare it against the original? That would mean the test doesn't just test printing, but reading also.

What do you think?

ak-coram commented 4 years ago

Fixed the test, please let me know if you prefer another approach.

plexus commented 4 years ago

looks good to me! thanks!