lua.js icon indicating copy to clipboard operation
lua.js copied to clipboard

Missing string functions + other fixes

Open florentpoujol opened this issue 11 years ago • 8 comments

Hey

I added the missing string functions : find, format, gsub, gmatch and match. All cases of pattern translation I encountered are working OK (yet there are probably many cases that won't).

I also fixed several other functions like ensure_arraymode(), pairs(), tonumber(), os.clock().

florentpoujol avatar Nov 25 '13 10:11 florentpoujol

Looks like %b is not supported. Am I right?

agladysh avatar Nov 27 '13 14:11 agladysh

You are right. I hadn't any use case for this pattern so I forgot about it. I am not sure yet how I could implement it but I will give it a proper look.

florentpoujol avatar Nov 27 '13 20:11 florentpoujol

NB: You could probably use a test suite, like fperrad/lua-TestMore, to detect such omissions.

agladysh avatar Nov 27 '13 20:11 agladysh

First, I want to thank you for contributing to lua.js. I hope you don't consider my comments too pedantic, I'm used to taking style pretty seriously (in fact I want to go through and correct my own style mistakes!). I am actually really grateful for your time and effort here.

I would recommend you try adding tests to your code as well (added by another contributor). It's pretty simple, just add new files to tests/, you can check out the files that are already there for examples.

Thanks again!

mherkender avatar Nov 28 '13 04:11 mherkender

Resume of what I did since the last batch of commits :

  • Fixed all styles issues as reported (braces, inline var declaration, etc...). (note that lua_add, lua_substract & Co are doing some inline variable declaration).
  • Improved some patterns, made unsupported patterns to call not_supported()
  • Added very partial support for the %b pattern (it's supported when it's the only item in a pattern).
  • Fixed a lot of bugs in the string functions

The cases where I had table.arraymode set to true but table.uints as object is fixed. It was due to another technology (CraftStudio's web player) that would add arraymode itself, yet left table.uints as an object.

I am new to node.js and I couldn't find how to run the tests in the "tests" folder. Which is why I hadn't them included in the pull request yet. Can you explain ?

Thanks !

florentpoujol avatar Dec 11 '13 16:12 florentpoujol

@florentpoujol looking at the package.json file, you should be able to run the tests with npm run test.

elisee avatar Jan 08 '14 11:01 elisee

@mherkender Hey, can you find time to review my latest commits ? Thanks !

florentpoujol avatar Jan 08 '14 14:01 florentpoujol

@mherkender Maybe it's time to do something about these improvements ?

They have been used without issues in several of my projects. The Travis CI build failed but apparently because of Node itself, not because a test failed. So I am not sure what I can do from here...

florentpoujol avatar Oct 18 '14 09:10 florentpoujol