Closed Kurvivor19 closed 8 years ago
Thank you for this work! Much appreciated.
Two things:
SENTINEL_NULL
in the elisp version unless we know that it greatly improves performance. SENTINEL_NULL
only exists in parinfer.js as a performance hack.Hello! my views on SENTINEL_NULL were formed when i've run (several times) into type error when nil was compared to a number. It seems to simplify checking in several places (because no need to check for nil before comparing) although at the same time it mandates longer checks (longer as in: longer lines of code)for the lack of value in others.
In regards to tests. At my end, so to say, only several tests fail, those are about 2 cross-mode preservation tests for paren mode. When i have asked Shaun about them, he replied with:
Those tests do not pass cross-mode preservation
Right, they should not pass. In fact, I do not run cross-mode preservation test if paren mode fails. You can see that I return before doing that check here: https://github.com/shaunlebron/parinfer/blob/6ba343a2cfcd308221faf2586fa0b12b41c162c7/lib/test/cases.js#L38-L44
More specifically, we were talking about these tests:
(foo
(foo
^ unclosed-paren
(defn foo
[arg
bar)
(defn foo
[arg
^ unclosed-paren
bar)
Everything else must be in order
2016-07-14 18:41 GMT+03:00 Chris Oakman notifications@github.com:
Thank you for this work! Much appreciated.
Two things:
- I would prefer to not use SENTINEL_NULL in the elisp version unless we know that it greatly improves performance. SENTINEL_NULL only exists in parinfer.js as a performance hack.
- Do you know why the Travis CI tests are failing?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/oakmac/parinfer-elisp/pull/11#issuecomment-232703757, or mute the thread https://github.com/notifications/unsubscribe/AInCDl9nq73JfGafdlnNj2-wLdRAlfwmks5qVli-gaJpZM4JMaRG .
It may be i do not know what i am talking about. Are Travis CI tests the same as tests from parinferlib (json files in cases folder)?
2016-07-14 21:44 GMT+03:00 Иван Трусков trus19@gmail.com:
Hello! my views on SENTINEL_NULL were formed when i've run (several times) into type error when nil was compared to a number. It seems to simplify checking in several places (because no need to check for nil before comparing) although at the same time it mandates longer checks (longer as in: longer lines of code)for the lack of value in others.
In regards to tests. At my end, so to say, only several tests fail, those are about 2 cross-mode preservation tests for paren mode. When i have asked Shaun about them, he replied with:
Those tests do not pass cross-mode preservation
Right, they should not pass. In fact, I do not run cross-mode preservation test if paren mode fails. You can see that I return before doing that check here: https://github.com/shaunlebron/parinfer/blob/6ba343a2cfcd308221faf2586fa0b12b41c162c7/lib/test/cases.js#L38-L44
More specifically, we were talking about these tests:
Unclosed Parens
(foo
(foo ^ unclosed-paren
(defn foo [arg bar)
(defn foo [arg ^ unclosed-paren bar)
Everything else must be in order
2016-07-14 18:41 GMT+03:00 Chris Oakman notifications@github.com:
Thank you for this work! Much appreciated.
Two things:
- I would prefer to not use SENTINEL_NULL in the elisp version unless we know that it greatly improves performance. SENTINEL_NULL only exists in parinfer.js as a performance hack.
- Do you know why the Travis CI tests are failing?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/oakmac/parinfer-elisp/pull/11#issuecomment-232703757, or mute the thread https://github.com/notifications/unsubscribe/AInCDl9nq73JfGafdlnNj2-wLdRAlfwmks5qVli-gaJpZM4JMaRG .
@oakmac that would be easier to add if you pushed a branch with the change and asked kurvivor to merge it into his PR branch
@Kurvivor19 Travis CI runs the JSON test cases against this library using the command here: https://github.com/oakmac/parinfer-elisp/blob/master/.travis.yml#L24.
@oakman
Certainly! These changes are no problem I will try to do something to get all tests to pass. It is well past time i have tried running tests in a script instead of trying to do that interactively
2016-07-16 8:40 GMT+03:00 Shaun LeBron notifications@github.com:
@oakmac https://github.com/oakmac that would be easier to add if you pushed a branch with the change and asked kurvivor to merge it into his PR branch
@Kurvivor19 https://github.com/Kurvivor19 Travis CI runs the JSON test cases against this library using the command here: https://github.com/oakmac/parinfer-elisp/blob/master/.travis.yml#L24.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/oakmac/parinfer-elisp/pull/11#issuecomment-233109922, or mute the thread https://github.com/notifications/unsubscribe-auth/AInCDt5HgTHFv2g2vkm4zLcT5lKZNxexks5qWG7lgaJpZM4JMaRG .
@oakmac Diff added, aye Tests all green now
Great :) I'm out of town this week; may be a while before I can take a look at this.
On Tuesday, July 19, 2016, Kurvivor19 notifications@github.com wrote:
@oakmac https://github.com/oakmac Diff added, aye Tests all green now
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/oakmac/parinfer-elisp/pull/11#issuecomment-233745201, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGAtQ-FBM_FR_T0EoyhVE0lUnWX4fbaks5qXSpMgaJpZM4JMaRG .
Thanks again for this work! :+1:
Update of parinferlib.el to reflect changes in original library (up to version 1.8.0) Improved testing facility