search

parinfer / parinfer.js

Let's simplify the way we write Lisp
https://shaunlebron.github.io/parinfer
MIT License
1.76k stars 40 forks source link

Fix for #173 #174

Closed cursive-ide closed 6 years ago

cursive-ide commented 7 years ago

This contains a fix for #173. I'd appreciate a review, since I'm not 100% sure this is the right fix. However it does fix that case and pass all existing tests.

The basic problem that when correcting the paren trail, the current line indent delta was not taken into account, only the opener indent delta. I must admit that I don't fully understand the subtleties of what getParentOpenerIndex is doing. Additionally, this is only taken into account when indenting, not dedenting - again, I'm not sure if this is the right solution, but it works for all the existing cases and #173.

I created a test case, but the current test case parser only allows a single change per line. I've left the case from #173 with the markdown for what the test case should look like in the sandbox.

shaunlebron commented 7 years ago

@cursive-ide I found a bug in this implementation. I committed the test case to master, which you can see here: https://github.com/shaunlebron/parinfer/commit/a66031f470ae423da087b9b6978f9662383f7cfa#diff-86a68219dcea971114a47a4315fce1ddR149

;; input
(foo (bar)
      baz)
     +
;; actual
(foo (bar)
      baz)

;; expected
(foo (bar
      baz))
shaunlebron commented 6 years ago

@cursive-ide I put together a commit here: https://github.com/shaunlebron/parinfer/commit/647b5881355e7c5d7c68096fb3cb42d6a554d3ad

It has a quick fix for the bug I mentioned, and I also started adding a lot comments around this function. I have two open questions marked in TODOs, one of which is related to Math.max which I believe you added to ignore dedentation, and the other related to what I believe is redundant indentation correction, but I have to continue investigating to remember what I was doing there.

There are more states to consider and I must continue looking after this Thanksgiving holiday, but I hope this helps if you want to take a second look.

cursive-ide commented 6 years ago

@shaunlebron Thanks! I'll investigate this next week too. Reading those comments, I'm glad it's not just me that found this tricky.

The Math.max thing is from this PR, and is probably incorrect. I noticed that all the failing cases shared a sign in common (I can't remember the details now), and that fixed it. But while testing various fixes for these problems, generally I could fix either indenting or dedenting, but not both. It's possible that's no longer required if you're now treating them separately.

shaunlebron commented 6 years ago

Closing in favor of #181