Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lua scanner -- Meta-commit. #22

Merged
merged 1 commit into from Jun 22, 2013
Merged

Conversation

Quintus
Copy link

@Quintus Quintus commented Apr 22, 2012

OK, here’s the "meta-commit" for the Lua scanner you requested.

But I don’t think it’s unusual to have a pull request containing multiple commits. In fact it’s what I see most times when I look at pull requests (GitHub even encourages to add even more commits to a pull requests after it has been opened).

Vale,
Quintus

This commit is a super-commit containing all the subcommits for
implementing the Lua scanner.
@Quintus
Copy link
Author

Quintus commented May 4, 2012

Any opinion? Plans for merging, rejecting?

Valete,
Quintus

@Quintus
Copy link
Author

Quintus commented May 31, 2012

If noone comments, I cannot improve the code. And as far as I know there’s always something to improve ;-)

Valete,
Quintus

@trans
Copy link

trans commented Jun 14, 2012

@Quintus I assume you've used your patch and know that it does the job. So that being so, @rubychan please merge!

@korny
Copy link
Member

korny commented Jun 19, 2012

Quintus: Sorry for taking so long to answer. I will look at it, Lua is on top of my wish list :)

@korny
Copy link
Member

korny commented Jun 19, 2012

Wow, your scanner seems almost perfect! It had no problems with the example file I threw at it (http://svn.rubychan.de/coderay-scanner-tests/trunk/lua/example.in.lua).

I fixed a Ruby 1.8 incompatibility (leading method dots…I was against that change in ruby-core ;-), and escaped the /{/ and /}/ regexps (confuses my editor).

Please run rake test:scanner:lua on the lua-scanner branch and take a look at the output (test/scanners/lua/example.actual.html). I have some minor questions about details like local variables and tables.

I wonder, why did you choose to use handle_state_* methods instead of the case @state / when :state approach all other scanners use? I tested the old variant (see the lua-scanner-faster branch) and it's about 20% faster on MRI 1.9.3. Is there a strong benefit I missed?

@ghost ghost assigned korny Jun 25, 2012
@Quintus
Copy link
Author

Quintus commented Jun 30, 2012

Quintus: Sorry for taking so long to answer.

You see I’m no better ;-). We all are forced to spent our time elsewhere...

Wow, your scanner seems almost perfect!

Thanks ;-). Given my initial motivation of writing this scanner (I justed wanted to highlight Lua code in a scripting manual generated via kramdown and was surprised that CodeRay had no Lua support) this surprises me as well. Of course I’ve tested some obscure code with the scanner (did you know all the ways you can write numbers in Lua?), but never with such a big file. Thanks for testing!

I fixed a Ruby 1.8 incompatibility (leading method dots…I was against that change in ruby-core ;-)

OK... I’ve stopped caring about 1.8 compatibility as this version will soon officially be discouraged.

escaped the /{/ and /}/ regexps (confuses my editor).

So your editor is not CodeRay-powered! What a shame! :-D

Please run rake test:scanner:lua on the lua-scanner branch

$ rake test:scanner:lua
svn up test/scanners
Updating 'test/scanners':
Revision 68.
/opt/rubies/ruby-1.9.3-p194/bin/ruby ./test/scanners/suite.rb lua

      !! Suite /home/quintus/Programmieren/Projekte/coderay/test/scanners/lua/suite.rb not found

I’ve then created the file with this content:

class Lua < CodeRay::TestCase
end

take a look at the output (test/scanners/lua/example.actual.html)

(540) [11:43:25 quintus@hades] ~/Programmieren/Projekte/coderay
$ diff -u test/scanners/lua/example.expected.html test/scanners/lua/example.actual.html 
(541) [11:44:24 quintus@hades] ~/Programmieren/Projekte/coderay
$

Not sure what you mean, looks quite identical to me. Could you be more specific?

I wonder, why did you choose to use handle_state_* methods instead of the case @State / when :state approach all other scanners use? I tested the old variant (see the lua-scanner-faster branch) and it's about 20% faster on MRI 1.9.3. Is there a strong benefit I missed?

Mh, I confess to not have thought that CodeRay not only aims to be the best, but also the fastest syntax highlighter out there in Ruby land... I simlpy dislike super-long case statements as they decrease readability which I favour over performance. Plus, it is easier to make recursive calls with methods rather than case statements. I can try to rewrite the scanner using case instead of send if you want.

Vale,
Quintus

@korny
Copy link
Member

korny commented Jul 1, 2012

I’ve stopped caring about 1.8 compatibility as this version will soon officially be discouraged.

I am prepared to drop 1.8 compat after CodeRay 1.1, but for the time being, I would like to keep it.

So your editor is not CodeRay-powered! What a shame! :-D

Technically, they have to be escaped. It seems TextMate is trying to be too clever when it parses RegExps.

!! Suite /home/quintus/Programmieren/Projekte/coderay/test/scanners/lua/suite.rb not found

Fixed. Forgot how to use SVN.

Not sure what you mean, looks quite identical to me. Could you be more specific?

It's identical because it creates *.actual.html on the first run. "expected" is what I got – I normally take the time to read/scan through it with my own eyes in the browser to spot errors. Then I fix them, run the suite again, and in the end, I can submit the good result as "expected".

So, in your case, look at either of the files and see if you can spot any problems.

I simlpy dislike super-long case statements as they decrease readability which I favour over performance.

I'm sure we could talk over this forever, exchanging valid points. Can you accept that I want to keep the codebase single-style? As in, all scanners should look/work the same, anticipating another batch rewrite in the future.

@korny
Copy link
Member

korny commented Jul 1, 2012

Plus, it is easier to make recursive calls with methods rather than case statements.

Not if you store the state in a variable anyway.

I can try to rewrite the scanner using case instead of send if you want.

I already did that in lua-scanner-faster :-)

@nathany
Copy link
Contributor

nathany commented Jul 2, 2012

One thing I'm wondering about is the addition of the "table" Style. Afaik, the tables in Lua are somewhat similar to the Arrays in PHP, supporting both arrays and hashes/associative arrays. Since styles are across languages in Coderay, I was wondering if there is a more appropriate name for the style itself? (or another style that could be reused)

@korny
Copy link
Member

korny commented Jul 3, 2012

I like the shades-of-blue trick. We could use that for JSON, too. Adding a new token kind is fine; maybe we find a more generic name?

@nathany
Copy link
Contributor

nathany commented Sep 11, 2012

Yah, the shades-of-blue trick is cool. It reminds me of how Xcode highlights indentation with shades of gray. Maybe it could be named a :dictionary or :dict though (which I believe is more general than hash map, and shorter than associative array).

It's been a while since I've looked at Lua, but I'm not seeing any obvious issues with the tokenizer in lua/example.expected.html. A few concerns that look related to the colors.

  • There appears to be a Unicode issue with Mark/Ovídio/Scuri on line ~1243. Funny characters.
  • Line 3648 looks a little funny, but I think it's just the colors selected and not a token issue. io.write('#include "il.h"\n\n\n')
  • Is it intentional that strings "..." '...' [[...]] are purple on grey when inside tables rather than the same as other strings, or is this just a side effect of the hsla stuff?

I've merged master and faster into lua-scanner, along with escaping some additional regex expressions. I'm working to convert a Lua blog from WordPress to NestaCMS, so I'll be referencing this branch.

Nathan.

@nathany
Copy link
Contributor

nathany commented Oct 28, 2012

Changed :table to :map, which is better for reuse in other languages, and won't be confused with :table based html rendering. 279348d

example.expected.raydebug needs to be updated

@nathany
Copy link
Contributor

nathany commented Oct 28, 2012

This article has some slightly bizarre code that is reporting as errors with the Lua scanner: http://www.luanova.org/porting-lua-to-risc-os.

@korny
Copy link
Member

korny commented Nov 11, 2012

Good finding! Can you add that code to the Lua test suite?

@ghost ghost assigned nathany Nov 11, 2012
@nathany
Copy link
Contributor

nathany commented Nov 11, 2012

I'm not actually sure if that is valid Lua code, I just assume it is because someone published it on my web site :-)

@korny
Copy link
Member

korny commented Nov 13, 2012

> luac -


-- Fred, tiny wimp program using error dialogue
require "wimp.task"
do
 local dim,! in riscos
luac: stdin:6: '<name>' expected near '!'

@ghost ghost assigned korny Dec 22, 2012
@Phrogz
Copy link

Phrogz commented Mar 4, 2013

FWIW I tried manually patching the current gem with the code from this commit, and the process using highlighting hung when I tried to highlight a Lua snippet. In case it was snippet-specific, here's what I was coloring:

require 'os'
local c = os.clock
function r1(i,n) return math.floor(i/n+0.5)*n      end
function r2(i,n) local m=n/2; return i+m - (i+m)%n end

local t=c(); for i=1,12345678 do r1(i,20) end; print(c()-t) --> 2.007
local t=c(); for i=1,12345678 do r2(i,20) end; print(c()-t) --> 1.45

@korny
Copy link
Member

korny commented Mar 5, 2013

Thanks! Can you post the error message?

@Phrogz
Copy link

Phrogz commented May 22, 2013

@korny I wish I could provide an error message, but there was none. Literally the process took 100% CPU and stayed there until I killed it. :/

@Phrogz
Copy link

Phrogz commented Jun 1, 2013

FWIW, I've written my own simple Lua scanner here: https://github.com/Phrogz/coderay/blob/master/lib/coderay/scanners/lua.rb

(I started out writing a recursive descent parser for Lua before realizing that the EBNF grammar supplied is left-recursive in many locations. So I went with the one huge regex approach that covers 90% of the formatting I wanted.)

@korny
Copy link
Member

korny commented Jun 22, 2013

example.expected.raydebug needs to be updated

Done!

Wow, activity after a few months ;) Can we still get this ready for CodeRay 1.1? I'm trying to…

@korny korny mentioned this pull request Jun 22, 2013
@korny
Copy link
Member

korny commented Jun 22, 2013

@Phrogz: I added your snipped to the test suite, it runs smoothly. Can't produce a bug.

@korny
Copy link
Member

korny commented Jun 22, 2013

@nathany:

There appears to be a Unicode issue with Mark/Ovídio/Scuri on line ~1243. Funny characters.

Yes, it was Latin-1 or something. Fixed it.

Line 3648 looks a little funny, but I think it's just the colors selected and not a token issue.

Not sure what you mean:
screenshot_01

Is it intentional that strings "..." '...' [[...]] are purple on grey when inside tables rather than the same as other strings, or is this just a side effect of the hsla stuff?

Funny, this is fixed by moving the .map styles before the .string styles (where they actually belong, alphabetically, now that they are named "map" instead of "table"). The CSS is really brittle; we have to fix this in the future. Possibly using .map > .content or something.

I prefer the red strings:
screenshot_01

This article has some slightly bizarre code that is reporting as errors with the Lua scanner: http://www.luanova.org/porting-lua-to-risc-os. I'm not actually sure if that is valid Lua code, I just assume it is because someone published it on my web site :-)

Lua says no:

lua: lua/fred.in.lua:4: '<name>' expected near '!'

Still, I added it as an example.

@korny
Copy link
Member

korny commented Jun 22, 2013

Yay, I have no issue left. The scanner is a bit slower than the Ruby scanner, but it's complex and handles complicated Lua code very well. We can optimize it later.

Thank you, @Quintus, for your awesome work on this. It's sad that my laziness prevented it from being merged earlier. CodeRay 1.1 will have great Lua support thanks to you!

@korny korny merged commit 501df76 into rubychan:master Jun 22, 2013
@nathany
Copy link
Contributor

nathany commented Jun 22, 2013

Thanks @korny

@korny korny mentioned this pull request Jun 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants