search

Engelberg / instaparse

Eclipse Public License 1.0
2.74k stars 149 forks source link

Fix #200. Corrects positioning of '^' when error line has tabs. (Supersedes PR #201) #204

Closed ema-fox closed 2 years ago

ema-fox commented 4 years ago

I fixed the pprint-failure-test as discussed in #201. While doing this I noticed that the original patch didn't work correctly in ClojureScript and fixed that too.

Engelberg commented 2 years ago

I'm embarrassed to say that I thought I incorporated and pushed out this pull request two years ago, but apparently I did not.

I ran the test suite against this pull request tonight, and got the following error:

FAIL in (pprint-failure-test) (failure_test.clj:24)
expected: (= (with-out-str (pprint-failure request)) (str "Parse error at line 3, column 16:\n" (:text request) "\n" "\t\t             ^\n" "Expected:\n" "\"A\"\n"))
  actual: (not (= "Parse error at line 3, column 16:\r\n\t\ti'm a sample error line with tabs.\r\n\t\t             ^\r\nExpected:\r\n\"A\"\r\n" "Parse error at line 3, column 16:\n\t\ti'm a sample error line with tabs.\n\t\t             ^\nExpected:\n\"A\"\n"))

I don't get this error with PR #201, but I do see that PR #201 generates some warnings under clojurescript, which probably relates to the issue you found under cljs. Can you clarify the situation for me? I'd love to get one of these pull requests included.

Engelberg commented 2 years ago

I'll add that I'm running the test suite on a Windows machine, in case that is a factor.

ema-fox commented 2 years ago

I had changed pprint-failure-test to do a string comparison instead of relying on a human to check that the output looks right. What I hadn't considered is that on windows newlines are "\r\n" instead of just "\n". I've now changed the test to use whatever newline style the platform uses so it should now pass on windows too.

Engelberg commented 2 years ago

Deployed as version 1.4.12. Thanks for the contribution!