On Nov 14, 2012, at 8:56 PM, Stefan Monnier wrote:

> Use a "json--" prefix, since it's a convention we use to express that
> something is internal.

Fixed.

> The first line of a docstring should "stand on it own", i.e. it
> shouldn't end in the middle of a sentence.
> Try C-u M-x checkdoc-current-buffer.

Ah, cool feature. Fixed.

> 
>> (defun json-encode-hash-table (hash-table)
>>   "Return a JSON representation of HASH-TABLE."
>> -  (format "{%s}"
>> +  (format (if json-encoding-pretty-print "{%s%s}" "{%s}")
> 
> Hmm... if json-encoding-pretty-print is nil, we still pass 2 args, and
> the second is always "", so we can always use "{%s%s}", right?

Not exactly, since json--encoding-current-indentation defaults to newline. I've tweaked this part to shift the complexity down into the second clause, however, instead of specifying 2 formats with a potentially unused second argument.

>> -  (concat "[" (mapconcat 'json-encode array ", ") "]"))
>> +  (if (and json-encoding-pretty-print
>> +           (> (length array) 0))
>> +      (concat
>> +       (let ((json-encoding-current-indentation
>> +              (concat json-encoding-current-indentation
>> +                      json-encoding-default-indentation)))
> 
> Use json--with-indentation here (even if it performs an extra redundant
> test of json-encoding-pretty-print).

Agreed, fixed.

I've also added an error handling clause to fix a bug I noticed where if you attempt to `json-pretty-print' some invalid JSON your text gets killed but formatting fails and it doesn't get replaced. Is there a more idiomatic way of doing this? I considered using copy-region-as-kill and then only killing the text if the call to json-encode succeeds, but that seemed more awkward.

Thanks for your help!

-Ryan