dancryer / PHPCI

PHPCI is a free and open source continuous integration tool specifically designed for PHP.
BSD 2-Clause "Simplified" License
2.42k stars 439 forks source link

PHPLint Task #111

Closed tobiastom closed 11 years ago

tobiastom commented 11 years ago

Not sure if it would duplicate existing functionality, but having a lint task that will simply check the syntax of files might be nice:

lint:
    directories:
        directoryA:
            - .php
            - .html
        directoryB:
            - *
    files:
        - lala.php
        - foo.html

The task would search inside directories for files ending with the given extensions. Each file would then be executed with php -l <file>.

I'm not happy with the structure of the definition, but like this it covers most features I would expect.

dancryer commented 11 years ago

Sounds like a good call to me. PHPMD and friends are a bit heavy for some people.

dongilbert commented 11 years ago

The php -l <file> command only checks for syntax issues, is that all you need? I think it would be useful, but was wondering if that's really all you are expecting from it.

tobiastom commented 11 years ago

Yes. That's all I would expect. It's pretty useful to at least basically validate templates files that are not unit tested.

Am 08.08.2013 um 17:20 schrieb Don Gilbert notifications@github.com:

The php -l command only checks for syntax issues, is that all need? I think it would be useful, but was wondering if that's really all you are expecting from it.

— Reply to this email directly or view it on GitHub.

dongilbert commented 11 years ago

OK. I'm working on a sample plugin now to accomplish this. I'll keep you posted.

dongilbert commented 11 years ago

Here's the start of my plugin: https://gist.github.com/dongilbert/6186988

In order to enable it, just put this in PHPCI/Plugin/PhpLint.php and then add the following to your phpci.yml

test:
    php_lint:
        directories:
            - "libraries"

Still working on supporting a files: param. At the moment it only checks .php.

Finally, once you enable it and run it on a build, you'll see the php lint output for every file it checks. I have it this way for now so that you can see it is actually working.

Let me know what you think.

tobiastom commented 11 years ago

I would strongly vote for a feature to make the extension customizable. Including *. Except from that I do like it.

Am 08.08.2013 um 20:00 schrieb Don Gilbert notifications@github.com:

Here's the start of my plugin: https://gist.github.com/dongilbert/6186988

In order to enable it, just put this in PHPCI/Plugins/PhpLint.php and then add the following to your phpci.yml

test: php_lint: directories:

  • "libraries" Still working on supporting a files: param. At the moment it only checks .php.

Finally, once you enable it and run it on a build, you'll see the php lint output for every file it checks. I have it this way for now so that you can see it is actually working.

Let me know what you think.

— Reply to this email directly or view it on GitHub.

dongilbert commented 11 years ago

You start getting "too many files open" errors when using RecursiveDirectoryIterator.

I'm using a RegexIterator to do matching on the items returned from the directory iterator. But how should it be done, do you think, to allow for a more flexible approach? I'm not sure about requiring the use of a manual regex to match.

dongilbert commented 11 years ago

And yes, it definitely needs more customization, but it's just a POC to see what you thought of it. Maybe @dancryer can work with what's there or help with the direction of the plugin?

dancryer commented 11 years ago

I would generally go for the simplest approach first, iterate through the directories and php -l anything that matches a given regex.

Once that's done, you should be able to see if there's a problem (performance wise.) Usually people will happily expand a plugin to add a specific feature they don't have, so I would release a basic one first and see where feedback takes it. :)

dongilbert commented 11 years ago

Ok. That's exactly what this one does. :) The only limitation is that the given regex might not meet all the needs, as stated by @tobiastom. Would it be acceptable to just have a list of file extensions to check? I could build a basic regex from that. The config could look like:

test:
    php_lint:
        directories:
            - libraries
        extensions:
            - php
            - html
dongilbert commented 11 years ago

Well, I could actually support the config markup that @tobiastom provided to begin with, as long as we limited the items under directoryA: to be just file extensions to check.

tobiastom commented 11 years ago

That would be totally fine for me.

Am 09.08.2013 um 15:47 schrieb Don Gilbert notifications@github.com:

Ok. That's exactly what this one does. :) The only limitation is that the given regex might not meet all the needs, as stated by @tobiastom. Would it be acceptable to just have a list of file extensions to check? I could build a basic regex from that. The config could look like:

test: php_lint: directories:

  • libraries extensions:
  • php
  • html — Reply to this email directly or view it on GitHub.
tobiastom commented 11 years ago

@dongilbert That would be even better. I did not really see that you changed it.

The idea should be (IMHO) to define directories to check, and define which extensions are inside this directory to be checked.

e.g. inside Templates/ there should "all" files be checked. Inside Classes/ only .php.

As a sidenode, not sure if you really should use the RegexIterator to check the extension, but this is an implementation detail. :)

dongilbert commented 11 years ago

Right, I'm going to change that up now that we're just checking extensions. For some reason I had assumed that you wanted to match arbitrary items. I'll update that when I get a chance.

dancryer commented 11 years ago

I think this is implemented using the parallel lint plugin now?

tobiastom commented 11 years ago

To be honest, I don't really understand how the ParallelLint task is supposed to work. I like the theory behind parallel running lints, but using did someone use the other lint task?

dancryer commented 11 years ago

I've added a new Lint plugin that works in a slightly more standard way... It also has no external dependencies. :)

tobiastom commented 11 years ago

Cool. Thanks! :)