whitequark / ast

A library for working with Abstract Syntax Trees.
MIT License
195 stars 25 forks source link

+ Add to_json method to AST::Node #20

Closed rattrayalex closed 6 years ago

rattrayalex commented 6 years ago

This is for consumption from the parser in https://github.com/whitequark/parser/pull/442

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-4.2%) to 87.963% when pulling ea6f250d4bf45f79799e6d006f1d901a16bacdbe on rattrayalex:patch-1 into 241cac1bea04fd7dfd9a83959713d7fa12b4e2ec on whitequark:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-4.2%) to 87.963% when pulling ea6f250d4bf45f79799e6d006f1d901a16bacdbe on rattrayalex:patch-1 into 241cac1bea04fd7dfd9a83959713d7fa12b4e2ec on whitequark:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-4.2%) to 87.963% when pulling ea6f250d4bf45f79799e6d006f1d901a16bacdbe on rattrayalex:patch-1 into 241cac1bea04fd7dfd9a83959713d7fa12b4e2ec on whitequark:master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.4%) to 92.593% when pulling 1cdc17493045f9b7c75c452c1942e2a0ea579826 on rattrayalex:patch-1 into 241cac1bea04fd7dfd9a83959713d7fa12b4e2ec on whitequark:master.

rattrayalex commented 6 years ago

Given that to_s and inspect both produce strings here, I figured to_json should both exist and produce a string.

I created a new to_sexp_array helper which is identical to to_a but includes the type as a symbol as the first element. I'm all ears for other ideas on the name.

I also added tests. I am relying on Travis to run them as I haven't set this up locally.

Thanks for the reviews so far! Appreciate your patience

whitequark commented 6 years ago

Get rid of to_json since it's redundant with JSON.dump and an implementation is trivial and noninvasive. to_sexp_array seems fine.

rattrayalex commented 6 years ago

Awesome, thanks! I also renamed to to_sexp_arr since that was what I found intuitive when writing tests. Hope that's okay.

whitequark commented 6 years ago

No, to_sexp_array was fine...

rattrayalex commented 6 years ago

K I'll move it back haha

rattrayalex commented 6 years ago

Sorry for all the back and forth, thanks so much for your patience!