wisp-lang / wisp

A little Clojure-like LISP in JavaScript
https://gozala.github.io/wisp/
Other
982 stars 68 forks source link

use commander.js for args handling; add --ast and --js-ast for debugging... #97

Closed xkxx closed 10 years ago

xkxx commented 10 years ago

Hi there,

This is the patch I emailed you about several days ago. It adds the following things:

The current interface is mostly compatible with the non-commander version. However, I wasn't able to verify every flag that existed because there is no documentation for them (one of the reason I created this patch)

Other thoughts:

File changes:

Gozala commented 10 years ago

Sorry I could not get to you earlier, I'm moving from SF to PDX today actually and have being very busy. I'll get to this once I'll get there, but here is few comments:

Here is how I use it binary without this patch:

echo "(print x)" | ./bin/wisp.js
console.log(x);
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImFub255bW91cy53aXNwIl0sIm5hbWVzIjpbImNvbnNvbGUiLCJsb2ciLCJ4Il0sIm1hcHBpbmdzIjoiQUFBQUEsT0FBQSxDQUFRQyxHQUFSLENBQU9DLENBQVAiLCJzb3VyY2VzQ29udGVudCI6WyIocHJpbnQgeClcbiJdfQ==

echo "(print x)" | ./bin/wisp.js --no-map
console.log(x);

echo "(print x)" | ./bin/wisp.js --compile --print forms                                                       ✭ ✱
[
  {
    "head": {
      "name": "print"
    },
    "tail": {
      "head": {
        "name": "x"
      },
      "tail": {},
      "length": 1
    },
    "length": 2
  }
]

echo "(print x)" | ./bin/wisp.js --compile --print ast
[
  {
    "op": "invoke",
    "callee": {
      "start": {
        "line": 0,
        "column": 0
      },
      "end": {
        "line": 0,
        "column": 9
      },
      "op": "member-expression",
      "computed": false,
      "form": {
        "head": {
          "name": "aget"
        },
        "tail": {
          "head": {
            "name": "console"
          },
          "tail": {
            "head": {
              "head": {
                "name": "quote"
              },
              "tail": {
                "head": {
                  "name": "log"
                },
                "tail": {},
                "length": 1
              },
              "length": 2
            },
            "tail": {},
            "length": 1
          },
          "length": 2
        },
        "length": 3
      },
      "target": {
        "start": {
          "line": 0,
          "column": 0
        },
        "end": {
          "line": 0,
          "column": 8
        },
        "op": "var",
        "type": "identifier",
        "form": {
          "name": "console"
        },
        "binding": {
          "op": "unresolved-binding",
          "type": "unresolved-binding",
          "identifier": {
            "type": "identifier",
            "form": {
              "name": "console"
            }
          },
          "start": {
            "line": 0,
            "column": 0
          },
          "end": {
            "line": 0,
            "column": 8
          }
        }
      },
      "property": {
        "start": {
          "line": 0,
          "column": 8
        },
        "end": {
          "line": 0,
          "column": 9
        },
        "op": "var",
        "type": "identifier",
        "form": {
          "name": "log"
        }
      }
    },
    "params": [
      {
        "start": {
          "line": 0,
          "column": 7
        },
        "end": {
          "line": 0,
          "column": 8
        },
        "op": "var",
        "type": "identifier",
        "form": {
          "name": "x"
        },
        "binding": {
          "op": "unresolved-binding",
          "type": "unresolved-binding",
          "identifier": {
            "type": "identifier",
            "form": {
              "name": "x"
            }
          },
          "start": {
            "line": 0,
            "column": 7
          },
          "end": {
            "line": 0,
            "column": 8
          }
        }
      }
    ],
    "form": {
      "head": {
        "name": "console.log"
      },
      "tail": {
        "head": {
          "name": "x"
        },
        "tail": {},
        "length": 1
      },
      "length": 2
    }
  }
]

echo "(print x)" | ./bin/wisp.js --compile --print js-ast
{
  "type": "Program",
  "body": [
    {
      "type": "ExpressionStatement",
      "expression": {
        "loc": {
          "start": {
            "line": 1,
            "column": 0
          },
          "end": {
            "line": 1,
            "column": 9
          }
        },
        "type": "CallExpression",
        "callee": {
          "loc": {
            "start": {
              "line": 1,
              "column": 0
            },
            "end": {
              "line": 1,
              "column": 9
            }
          },
          "type": "MemberExpression",
          "computed": false,
          "object": {
            "loc": {
              "start": {
                "line": 1,
                "column": 0
              },
              "end": {
                "line": 1,
                "column": 8
              }
            },
            "type": "Identifier",
            "name": "console"
          },
          "property": {
            "loc": {
              "start": {
                "line": 1,
                "column": 8
              },
              "end": {
                "line": 1,
                "column": 9
              }
            },
            "type": "Identifier",
            "name": "log"
          }
        },
        "arguments": [
          {
            "loc": {
              "start": {
                "line": 1,
                "column": 7
              },
              "end": {
                "line": 1,
                "column": 8
              }
            },
            "type": "Identifier",
            "name": "x"
          }
        ]
      },
      "loc": {
        "start": {
          "line": 1,
          "column": 0
        },
        "end": {
          "line": 1,
          "column": 9
        }
      }
    }
  ],
  "loc": {
    "start": {
      "line": 1,
      "column": 0
    },
    "end": {
      "line": 1,
      "column": 9
    }
  }
}

I don't mind if you add commander as a dependency as long as all it's backwards compatible, meaning this commands should produce same results. I am afraid I cant look at the patch yet, but will do this weekend.

Thanks.

xkxx commented 10 years ago

Sorry about the delay. The latest commit implements the --output flag which causes the output to be written to the specified directory. --no-ast now works, and --print is now backward-compatible.

Some thoughts:

xkxx commented 10 years ago

To add to my last comment, the debug output would be like this:

> echo "(print x)" | ./bin/wisp.js --compile --print forms
(console.log x)

> echo "(print x)" | ./bin/wisp.js --compile --print ast
[{"op" "invoke",
    "callee" {"start" {"line" 0,
                       "column" 0},
              "end" {"line" 0,
                     "column" 9},
              "op" "member-expression",
              "computed" false,
              "form" (aget console (quote log)),
              "target" {"start" {"line" 0,
                                 "column" 0},
                        "end" {"line" 0,
                               "column" 8},
                        "op" "var",
                        "type" "identifier",
                        "form" console,
                        "binding" {"op" "unresolved-binding",
                                   "type" "unresolved-binding",
                                   "identifier" {"type" "identifier",
                                                 "form" console},
                                   "start" {"line" 0,
                                            "column" 0},
                                   "end" {"line" 0,
                                          "column" 8}}},
              "property" {"start" {"line" 0,
                                   "column" 8},
                          "end" {"line" 0,
                                 "column" 9},
                          "op" "var",
                          "type" "identifier",
                          "form" log,
                          "binding" nil}},
    "params" [{"start" {"line" 0,
                        "column" 7},
               "end" {"line" 0,
                      "column" 8},
               "op" "var",
               "type" "identifier",
               "form" x,
               "binding" {"op" "unresolved-binding",
                          "type" "unresolved-binding",
                          "identifier" {"type" "identifier",
                                        "form" x},
                          "start" {"line" 0,
                                   "column" 7},
                          "end" {"line" 0,
                                 "column" 8}}}],
    "form" (console.log x)}]

> echo "(print x)" | ./bin/wisp.js --compile --print js-ast
{"type" "Program",
   "body" [{"type" "ExpressionStatement",
            "expression" {"loc" {"start" {"line" 1,
                                          "column" 0},
                                 "end" {"line" 1,
                                        "column" 9}},
                          "type" "CallExpression",
                          "callee" {"loc" {"start" {"line" 1,
                                                    "column" 0},
                                           "end" {"line" 1,
                                                  "column" 9}},
                                    "type" "MemberExpression",
                                    "computed" false,
                                    "object" {"loc" {"start" {"line" 1,
                                                              "column" 0},
                                                     "end" {"line" 1,
                                                            "column" 8}},
                                              "type" "Identifier",
                                              "name" "console"},
                                    "property" {"loc" {"start" {"line" 1,
                                                                "column" 8},
                                                       "end" {"line" 1,
                                                              "column" 9}},
                                                "type" "Identifier",
                                                "name" "log"}},
                          "arguments" [{"loc" {"start" {"line" 1,
                                                        "column" 7},
                                               "end" {"line" 1,
                                                      "column" 8}},
                                        "type" "Identifier",
                                        "name" "x"}]},
            "loc" {"start" {"line" 1,
                            "column" 0},
                   "end" {"line" 1,
                          "column" 9}}}],
   "loc" {"start" {"line" 1,
                   "column" 0},
          "end" {"line" 1,
                 "column" 9}}}
Gozala commented 10 years ago
The current --print flag is kind of ambiguous. Wouldn't it be better to call it --debug or something?

In my mind --print tells compiler what to print which is either js-code, js-ast or ast maybe more stuff in the future. I'm not sure --debug would be a better name for that even if you use printed output to debug a problem. Does this makes sense ?

Gozala commented 10 years ago

I think there should be a way of printing the macro-expanded lisp forms for debugging purposes. --print forms does it, but the output is json and not as nice-looking.

I dont mind adding --print expansion to print macro expaneded forms, although that would require some work as compiler does macro expansion along at the analyzer phase in one pass over AST, which I would like to keep as additional walk would increase a compilation time. That being said separate functionality could be exposed to just do that.

Gozala commented 10 years ago

--no-ast now works

I presume you meant --no-map instead.

Gozala commented 10 years ago

Everything seems fine here to me with except --output option that I think is redundant both windows and posix have a way to write stdout to a file > ./my/file I don't see a point in adding option to do just that.

xkxx commented 10 years ago

I'm using the coffeescript/livescript compiler as a point of reference here. In their implementation, --print print out the compiled JavaScript rather than directing writing it to files. --print is mainly used for debugging and thus shouldn't have a common name like this to confuse users, I think.

Also, The processed AST returned from the compiler is already macro-expanded. In my current working repo I have these lines:

                  (= channel :forms) (reduce (fn [result item]
                                               (str result (pr-str (:form item)) "\n"))
                                             "" (:ast output))

So that when you do --print forms it prints macro-expanded forms.

Regarding --output: Again, this feature is present in coffeescript compiler. #80 also requested this feature. It helps makefile scripting since you don't have to manually select and change file extension for each compiled file, but rather send all the source files to compiler in one go. The patch also allows compiling multiple files at once, like wisp -c 1.wisp 2.wisp -o dist/

davidchambers commented 10 years ago

--output […] is present in coffeescript compiler.

It is, though not in CoffeeScriptRedux. Michael omitted the option for the reason @Gozala gave above.

It helps makefile scripting since you don't have to manually select and change file extension for each compiled file

I don't think you're taking full advantage of Make. You could do something like this:

SRC = $(shell find src -name '*.wisp')
LIB = $(patsubst src/%.wisp,lib/%.js,$(SRC))

.PHONY: all
all: $(LIB)

lib/%.js: src/%.wisp
    mkdir -p '$(@D)'
    wisp -c '$<' >'$@'
Gozala commented 10 years ago

On Wednesday, May 7, 2014, xkxx notifications@github.com wrote:

I'm using the coffeescript/livescript compiler as a point of reference here. In their implementation, --print print out the compiled JavaScript rather than directing writing it to files.

Well without your change it does print out thing you tell it to :) Maybe --format can be a better name then.

--print is mainly used for debugging and thus shouldn't have a common name like this to confuse users, I think.

Also, The processed AST returned from the compiler is already macro-expanded. In my current working repo I have these lines:

              (= channel :forms) (reduce (fn [result item]
                                           (str result (pr-str (:form item)) "\n"))
                                         "" (:ast output))

So that when you do --print forms it prints macro-expanded forms.

Oh you use forms put onto ast nodes that are indeed expanded, thats smart, I have not realized expanded forms were already available.

Never the less could you please that under seperate --expansion flag cause I use --forms to track down reader bugs

Regarding --output: Again, this feature is present in coffeescript compiler. #80 https://github.com/Gozala/wisp/issues/80 also requested this feature. It helps makefile scripting since you don't have to manually select and change file extension for each compiled file, but rather send all the source files to compiler in one go. The patch also allows compiling multiple files at once, like wisp -c 1.wisp 2.wisp -o dist/

I still hate it, but fine I buy it ;)

— Reply to this email directly or view it on GitHubhttps://github.com/Gozala/wisp/pull/97#issuecomment-42445079 .

Regards

Irakli Gozalishvili Web: http://www.jeditoolkit.com/

Gozala commented 10 years ago

@xkxx so I have complete the review of the changes, could you please address them and then I'm happy to land this.

Also after @davidchambers comment in regards to changes to a coffeescript I'm a lot more resistant to an --output option & would really prefer users to simplify makefiles as he has suggested or just let them define their own frontends to a compiler.

Gozala commented 10 years ago

One more thing I have realized was that in your case --print forms is less useful than pr-str version of it. I do think they both have they're use cases. So I was thinking that it would be best to change compiler frontend to allow passing in not only wisp code but any intermediate form:

echo "(hello world)" | wisp --output forms
[
  {
    "head": {
      "name": "hello"
    },
    "tail": {
      "head": {
        "name": "world"
      },
      "tail": {},
      "length": 1
    },
    "length": 2
  }
]

echo "(hello world)" | wisp --output forms | wisp --input forms --output code
(hello world)
xkxx commented 10 years ago

There is one thing the compiler can't do without --output, to chain compilations together like this: wisp -c file1.wisp file2.wisp file3.wisp -o ./dist

And also it just makes things easier in terminal.

Changing JSON.stringify to pr-str has one added benefit -- it makes output more concise and human readable (JSON is not as readable as lisp data structures). Compare

> echo "(print x)" | wisp --compile --print ast
[
  {
    "op": "invoke",
    "callee": {
      "start": {
        "line": 0,
        "column": 0
      },
      "end": {
        "line": 0,
        "column": 9
      },
      "op": "member-expression",
      "computed": false,
      "form": {
        "head": {
          "name": "aget"
        },
        "tail": {
          "head": {
            "name": "console"
          },
          "tail": {
            "head": {
              "head": {
                "name": "quote"
              },
              "tail": {
                "head": {
                  "name": "log"
                },
                "tail": {},
                "length": 1
              },
              "length": 2
            },
            "tail": {},
            "length": 1
          },
          "length": 2
        },
        "length": 3
      },
      "target": {
        "start": {
          "line": 0,
          "column": 0
        },
        "end": {
          "line": 0,
          "column": 8
        },
        "op": "var",
        "type": "identifier",
        "form": {
          "name": "console"
        },
        "binding": {
          "op": "unresolved-binding",
          "type": "unresolved-binding",
          "identifier": {
            "type": "identifier",
            "form": {
              "name": "console"
            }
          },
          "start": {
            "line": 0,
            "column": 0
          },
          "end": {
            "line": 0,
            "column": 8
          }
        }
      },
      "property": {
        "start": {
          "line": 0,
          "column": 8
        },
        "end": {
          "line": 0,
          "column": 9
        },
        "op": "var",
        "type": "identifier",
        "form": {
          "name": "log"
        }
      }
    },
    "params": [
      {
        "start": {
          "line": 0,
          "column": 7
        },
        "end": {
          "line": 0,
          "column": 8
        },
        "op": "var",
        "type": "identifier",
        "form": {
          "name": "x"
        },
        "binding": {
          "op": "unresolved-binding",
          "type": "unresolved-binding",
          "identifier": {
            "type": "identifier",
            "form": {
              "name": "x"
            }
          },
          "start": {
            "line": 0,
            "column": 7
          },
          "end": {
            "line": 0,
            "column": 8
          }
        }
      }
    ],
    "form": {
      "head": {
        "name": "console.log"
      },
      "tail": {
        "head": {
          "name": "x"
        },
        "tail": {},
        "length": 1
      },
      "length": 2
    }
  }
]

And

 > echo "(print x)" | ./bin/wisp.js --compile --print ast
[{"op" "invoke",
    "callee" {"start" {"line" 0,
                       "column" 0},
              "end" {"line" 0,
                     "column" 9},
              "op" "member-expression",
              "computed" false,
              "form" (aget console (quote log)),
              "target" {"start" {"line" 0,
                                 "column" 0},
                        "end" {"line" 0,
                               "column" 8},
                        "op" "var",
                        "type" "identifier",
                        "form" console,
                        "binding" {"op" "unresolved-binding",
                                   "type" "unresolved-binding",
                                   "identifier" {"type" "identifier",
                                                 "form" console},
                                   "start" {"line" 0,
                                            "column" 0},
                                   "end" {"line" 0,
                                          "column" 8}}},
              "property" {"start" {"line" 0,
                                   "column" 8},
                          "end" {"line" 0,
                                 "column" 9},
                          "op" "var",
                          "type" "identifier",
                          "form" log,
                          "binding" nil}},
    "params" [{"start" {"line" 0,
                        "column" 7},
               "end" {"line" 0,
                      "column" 8},
               "op" "var",
               "type" "identifier",
               "form" x,
               "binding" {"op" "unresolved-binding",
                          "type" "unresolved-binding",
                          "identifier" {"type" "identifier",
                                        "form" x},
                          "start" {"line" 0,
                                   "column" 7},
                          "end" {"line" 0,
                                 "column" 8}}}],
    "form" (console.log x)}]
xkxx commented 10 years ago

The latest commit does not use pr-str for --print ast and --print js-ast. If you want to see the output above, changing JSON.stringify to pr-str would do.

davidchambers commented 10 years ago

There is one thing the compiler can't do without --output, to chain compilations together like this: wisp -c file1.wisp file2.wisp file3.wisp -o ./dist

And also it just makes things easier in terminal.

This is true, but I don't think this is a use case for which we should optimize. Better to teach people about Make so that all one needs to run is make.

Gozala commented 10 years ago

Given the disagreement on --output even in the pull request, I would really prefer to leave it off for now. We can keep discussion open on this in a separate thread. Also I think simple alias in bash should be able to address wisp -c file1.wisp file2.wisp file3.wisp -o ./dist use case, which is probably better than reinventing the tool already available in terminal.

xkxx commented 10 years ago

The latest commit disables --output for now. I'll open a new issue about that later.

Gozala commented 10 years ago

I am sorry to keep discussion going on & on, but could you please remove stuff that isn't used as of now, I have put comments for each such case.

xkxx commented 10 years ago

Done.

Gozala commented 10 years ago

Thanks a lot!

Gozala commented 10 years ago

I am afraid this broke the build :( https://travis-ci.org/Gozala/wisp/jobs/24670580

xkxx commented 10 years ago

On line 83, wisp.wisp, (run options.args) should be (run (get options.args 0)). I didn't test this, sorry about that:(

Gozala commented 10 years ago

I have fixed regression & had to change few more things as I did noticed regressions in a usage of a CLI.