luaparse icon indicating copy to clipboard operation
luaparse copied to clipboard

Added initial support for a P8 (Pico-8) dialect

Open whizzter opened this issue 4 years ago • 2 comments

The changes so far are the addition of a new P8 dialect with bitwise operators, compound assignments and a != alias for ~=

There are a also some other shorthands for the P8 dialect listed on https://www.lexaloffle.com/pico-8.php?page=manual but I didn't run into issues with those yet in my project and didn't have time yet to check how to modify the parser (I'll probably run into those later on though but could be good to have the initial parts upstreamed?)

whizzter avatar Sep 14 '20 22:09 whizzter

Not rejecting yet, but I am definitely not going to merge this as-is.

  • I am split on whether I am willing to support a proprietary dialect in the first place.
  • As admitted above, some features of the dialect are still missing; I would rather not add features that only partially work.
  • Have you verified that this is indeed how the tokenizer works (e.g. does a + = b not work)?
  • Linter complains about style, you should fix that.
  • The test cases are missing. Add some in test/scaffolding/, then run make scaffold-tests. I have been maintaining 100% coverage* for some time, and would prefer to keep it that way.

* other than in portability code

fstirlitz avatar Sep 15 '20 08:09 fstirlitz

  • I am split on whether I am willing to support a proprietary dialect in the first place.

Well there's PicoLove that's an opensource impl for porting and I'm using this for a small experiment runtime (No idea how useful it'll end up but more open support wouldn't hurt?)

  • As admitted above, some features of the dialect are still missing; I would rather not add features that only partially work.

Agreed, already ran into more missing features so i could def re-submit later if you'd prefer that.

  • Have you verified that this is indeed how the tokenizer works (e.g. does a + = b not work)?

Not yet, will verify.

  • Linter complains about style, you should fix that.
  • The test cases are missing. Add some in test/scaffolding/, then run make scaffold-tests. I have been maintaining 100% coverage* for some time, and would prefer to keep it that way.

Yeah didn't notice the existence of these 2 before submitting and noticing the CI run.

whizzter avatar Sep 15 '20 18:09 whizzter