search

markwatkinson / luminous

Accurate and powerful syntax highlighting library
http://luminous.asgaard.co.uk
GNU Lesser General Public License v2.1
50 stars 5 forks source link

PSR Conversion #34

Closed J5lx closed 9 years ago

J5lx commented 9 years ago

Hi, it’s me again! After almost one year I finally managed to convince myself of converting the last few files to follow PSR-2 and PSR-4. Please note though that for now I have only converted the codebase of the actual syntax highlighting stuff; I haven’t touched things like docs, tests, and examples. Some other noteworthy aspects:

Resolves #31.

J5lx commented 9 years ago

Oh, one more thing: Since the naming conventions changed this breaks BC and needs a major version bump if you adhere to semver.

markwatkinson commented 9 years ago

Wow - thanks for this.

There is a lot here and I probably won't get the chance to look through this until next week, but I will definitely go through it properly next week, so if you want to keep editing please feel free.

On 4 August 2015 at 08:01, J5lx notifications@github.com wrote:

Oh, one more note: Since the naming conventions changed this breaks BC and needs a major version bump if you adhere to semver.

— Reply to this email directly or view it on GitHub https://github.com/markwatkinson/luminous/pull/34#issuecomment-127502448 .

markwatkinson commented 9 years ago

I have pulled this into the new 'psr-conversion' branch rather than master, and fixed a couple of very minor things I found.

Since PSR-1 requires files not to declare symbols and cause side effects (such as requireing files) I inlined the function lists in the scanners. I’m still looking for a better solution to this; any ideas are welcome.

Ok, if I understand this correctly you're referring to the long arrays in the scanners. For example in JavaScanner.php there are about 2.7k lines of keywords/functions/etc. I'm not really happy about this because it makes the code quite unfriendly to work with.

So, what can we do?

Let's say we introduce, e.g. JavaScannerData which is a new class that just has a bunch of static properties which are the long array definitions, and then JavaScanner.php uses JavaScannerData and accesses its properties. So we keep the long definitions separate from any actual scanning logic. Does that work?

J5lx commented 9 years ago

That does in fact sound like a reasonable solution. I personally would go for more specific naming like XyzKeywords or something instead of XyzData, but that's just preference. However I'd definitely put all those utility classes into their own namespace, Luminous\Scanners\Keywords or Luminous\Scanners\Data or whatever those classes are going to be named like in the end.

Oh, I'd also go for class constants instead of properties, since those keyword lists are not meant to be modified (they were also immutable even before I touched a single line of the code).

markwatkinson commented 9 years ago

Yep - agreed with all of that.

On 10 August 2015 at 18:32, J5lx notifications@github.com wrote:

That does in fact sound like a reasonable solution. I personally would go for more specific naming like XyzKeywords or something instead of XyzData, but that's just preference. However I'd definitely put all those utility classes into their own namespace, Luminous\Scanners\Keywords or Luminous\Scanners\Data or whatever those classes are going to be named like in the end.

Oh, I'd also go for class constants instead of properties, since those keyword lists are not meant to be modified (they were also immutable even before I touched a single line of the code).

— Reply to this email directly or view it on GitHub https://github.com/markwatkinson/luminous/pull/34#issuecomment-129541401 .

J5lx commented 9 years ago

I finally got around to doing this - After being on vacation and getting caught up in work afterwards I almost forgot about it. For the moment I had to use a little workaround to make it work in 7.0.0RC2 (const TYPES= ... instead of const TYPES = ... - pretty weird), but very soon I'm going to check wether that's still needed in RC3.

markwatkinson commented 9 years ago

Thanks for doing this, I'll get some time to look at it in the next few days.

Is the PSR conversion now complete from your perspective?

J5lx commented 9 years ago

Yes, I think I’m pretty much done here. However I’d like to do some other stuff as well before this gets incorporated into a new version (e.g. #35, maybe also #33 and the known issues I mentioned in #32).

markwatkinson commented 9 years ago

On my machine I am getting a syntax error on some files, e.g.:

$ php tests/unit/syntax.php
PHP Fatal error:  Arrays are not allowed in class constants in /home/mark/projects/luminous/src/Luminous/Scanners/Keywords/CKeywords.php on line 63
$ php -v
PHP 5.5.9-1ubuntu4.11 (cli) (built: Jul  2 2015 15:23:08) 
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2014 Zend Technologies
    with Zend OPcache v7.0.3, Copyright (c) 1999-2014, by Zend Technologies

Changing from const KEYWORDS = array(... to static $KEYWORDS = array( seems to be OK. According to https://stackoverflow.com/questions/1290318/php-constants-containing-arrays const arrays will work since 5.6 so since this is quite easy for us to change I'd prefer to keep compatibility with pre 5.6 versions.

J5lx commented 9 years ago

Oh yeah, I forgot about that. I’m almost always using the latest PHP version, and sometimes I lose track of when certain features were introduced.