Modernize and fix #83 #84
Conversation
|
Hey @robertleeplummerjr, thanks for the PR! Will try to find some time over the next week or so to check out these changes. |
|
Hi @robertleeplummerjr, I finally got a chance to look through your changes a bit. You've been busy! There's about 25 failures when I run I don't see any new or updated tests for this work, which would make it difficult to ensure your changes continue to work as expected. How have you been testing your changes? |
|
I was testing using manual because of how in-depth the tests were. We diverged enough from the original codebase that we started this: https://github.com/SaramaJS/sarama.js and are testing using a simplified method, mocha and should.js with acorn and comparing side by side a python string to it's JavaScript equivalent. By using mocha, we have better ide compatibility for debugging in real time. This had made development time quite a bit faster, but we didn't want to superimpose our own ideology on your project. One of the things we descided on for scalability was to make the parser do as little as possible in terms of rewriting for compatibility to run in node or web. This way we can focus just on parsing and not environment. The environment "casting" (from pythons ast to one acceptable by node) would be done in a different project, one focused on simple rewrites to different parts of the ast. By abstracting the two portions of the project development time for this project will drop, befause the project is responsible for less, and we'd gain more thorough unit testing. That was the aim anyway. We'd be happy to drop the project and merge these into your project if you wanted the changed, but we just didn't want to step on toes. |
|
Here is the branch the work has been done on. Unit tests currently pass. SaramaJS/sarama.js#11 |
2 participants
No description provided.