search

oakmac / sublime-text-parinfer

Parinfer plugin for Sublime Text
ISC License
57 stars 8 forks source link

Update parinfer to newest reference version - 3.12.0 #38

Closed oconnor0 closed 4 years ago

oconnor0 commented 4 years ago

The version of parinfer used here and in parinfer.py is much older, as far as I can tell, then the most recent reference in parinfer.js which is 3.12.0. Given the bug fixes and what not, it would be good to incorporate them into sublime-text-parinfer to improve its use.

I have performed a direct translation of parinfer.js to parinfer.py at oconnor0/parinfer.py/reference and then performed a few optimizations at oconnor0/parinfer.py/optimize.

When I copied the updated parinfer.py into my clone of this repo, all of the functionality I would expect is present. I was concerned about performance issues as the runtime between the 0.7.0 and 3.12.0 Python implementations that the new version is 3-4x slower, even after some optimizations, but haven't encountered performance issues with it yet.

I did not incorporate the dynamic comment character changes as they weren't relevant to me, and I wanted to make sure I had a faithful translation of parinfer.js first.

I also found at least one improvement/bug fix in the plugin portion of this package.

My question is, essentially, how would you like to proceed? I can open a couple of merge requests for here and parinfer.py. Or something else?

oakmac commented 4 years ago

Wonderful news! It has been a lagging task for me to update parinfer.py and this extension to the latest. Integrating Smart Mode with Sublime Text would be really great to have.

I think you are on the right track. Roughly speaking, the process should be:

1) update parinfer.py to the latest parinfer.js, make sure it passes the same test suite as parinfer.js v3.12.0 2) update this extension to use the new parinfer.py package, hopefully adding smart mode + your bug fixes as well. Fair warning: smart mode integration can be pretty tricky.

If you open a PR against the parinfer.py repo and this repo I would be happy to review and merge!

Another possible avenue to consider would be using the parinfer-rust package by @eraserhd. I don't know anything about binding python with rust, but I suspect it is possible. I think there is value in having a pure Python parinfer implementation, if only to make integration easier.

oconnor0 commented 4 years ago

Excellent. I will try to get to that this weekend.