> Hi Stevan,
> 
> I am going to be a bit more picky this time around, but please do not take it
> too bad.
> 
> > === modified file 'README'
> ...
> > +Unit tests
> > +----------
> 
> I don't think it's that interesting that these are unit-tests.  FWIW, we can
> perfectly well have Python unit tests in this same branch, so this is a wrong
> title in my opinion.
> 
> Relevant title would be "Tests for PHP license-matching logic" or similar.
> 
> You should also make this a subsection of "Tests" section below it.  This
> README file is using ReST, so all it takes to get a "subsection" is to use a
> different underlying character (the file already uses "." above).
> 
> > +
> > +There's currently only one unit test file, LicenseHelperTest.php under
> testing directory. You first need to install the phpunit package from ubuntu
> repos:
> 
> It would be good to follow the 79-character limit in the README as well.  It
> was probably me who messed this up originally, so let's not worry about it
> right now.
> 
> > +$ sudo apt-get install php-pear
> > +$ sudo pear config-set auto_discover 1
> > +$ sudo pear install pear.phpunit.de/PHPUnit
> 
> Generic instructions should be "sudo apt-get install phpunit".  The fact that
> phpunit package is broken in current Ubuntu/Debian releases is not something
> we want to rely on.  FWIW, Ubuntu 10.04 (previous LTS, which most of our
> servers still run and will run for another 6 months) has a working phpunit
> package (it doesn't seem to have assertNotEmpty method, though).
> 
> > +
> > +Then to run the test from the 'testing' directory:
> > +$ cd testing/
> > +$ phpunit LicenseHelperTest
> > +
> > +In case the tests fail with PHP_CodeCoverage error, do the following to
> install the phpunit properly:
> > +
> > +$ sudo apt-get remove phpunit
> > +$ sudo apt-get upgrade pear
> > +$ sudo pear channel-discover pear.phpunit.de
> > +$ sudo pear channel-discover pear.symfony-project.com
> > +$ sudo pear channel-discover components.ez.no
> > +$ sudo pear update-channels
> > +$ sudo pear upgrade-all
> > +$ sudo pear install --alldeps phpunit/PHPUnit
> > +$ sudo apt-get install phpunit
> 
> Your former instructions solve that problem as well.  You should list either
> one or the other, not both.  You should also mention what we know of the
> problem (i.e. "recent (as of 2012-05-08) Ubuntu/Debian releases have a broken
> phpunit package and thus require installation in a different manner").
> 
> Also, please use some indentation for commands like it's already done in the
> 'Tests' section for testr ones.
> 
> 
> > === added file 'licenses/LicenseHelper.php'
> > --- licenses/LicenseHelper.php        1970-01-01 00:00:00 +0000
> > +++ licenses/LicenseHelper.php        2012-05-07 15:13:20 +0000
> > @@ -0,0 +1,104 @@
> > +<?php
> > +
> > +class LicenseHelper
> 
> Trailing whitespace, please remove it.  If you are using Emacs (I believe you
> are), you can have it highlighted by adding the following to your .emacs:
> 
>   (setq show-trailing-whitespace t)
> 
> Note, I understand you have mostly copied the code over in this file, so that
> makes it harder for me to comment on it.  Thus, I'll focus on style first.
> 
> > +{
> > +
> > +  // Get list of files into array to process them later.
> > +  // Used to find special licenses and dirs with only subdirs.
> > +  public static function checkFile($fn)
> > +  {
> > +    if (is_file($fn) or is_link($fn)) {
> > +      return true;
> > +    }
> > +    return false;
> > +  }
> 
> Please use four-space indentation in this file, or at least follow the
> existing (but otherwise bad) tab-indentation style: we want the entire project
> to use similar style for all of code, and Python code is already using
> 4-spaces.
> 
> I see there is also http://pear.php.net/manual/en/standards.php which seems to
> be what PEP8 is for Python.  We should start a discussion on whether that's
> what we want to follow and how should we adapt it to suit our needs and cross-
> language nature.
> 
> In particular, http://pear.php.net/manual/en/standards.indenting.php agrees
> with what I am proposing here.
> 
> > +
> > +  public static function getFilesList($dirname)
> > +  {
> > +    if (!is_dir($dirname)) {
> > +      throw new InvalidArgumentException('Method argument should be a
> directory path');
> > +    }
> 
> 79-character line limit, I know it wasn't respected originally, please fix it
> here.
> 
> > +
> > +    $files = array();
> 
> Another trailing whitespace.
> 
> > +    if ($handle = opendir($dirname)) {
> > +      while ($handle && false !== ($entry = readdir($handle))) {
> > +     if ($entry != "." && $entry != ".." && !is_dir($dirname."/".$entry) &&
> $entry != "HEADER.html") {
> 
> More overflow.
> 
> ...
> > +
> > +  // Get array of file name and extension from full filename.
> > +  public static function splitFilename($filename)
> > +  {
> > +    $pos = strpos($filename, '.');
> > +    if ($pos === false) { // dot is not found in the filename
> > +      return array($filename, ''); // no extension
> > +    } else {
> > +      $basename = substr($filename, 0, $pos);
> > +      $extension = substr($filename, $pos+1);
> > +      return array($basename, $extension);
> > +    }
> > +  }
> 
> I thought we agreed to get rid of this function?
> 
> > +  {
> > +    if (!empty($fl)) {
> > +      foreach ($fl as $f) {
> > +     if (preg_match($pattern, $f, $matches)) {
> > +       return $f;
> > +     }
> 
> Indentation here seems entirely broken.
> 
> > === modified file 'licenses/license.php'
> 
> Changes here look good.
> 
> 
> > === added file 'testing/LicenseHelperTest.php'
> 
> For this new file, please apply the coding style as suggested above: 4 space
> indentation, 79 character line limit, no trailing whitespaces.
> 
> > --- testing/LicenseHelperTest.php     1970-01-01 00:00:00 +0000
> > +++ testing/LicenseHelperTest.php     2012-05-07 15:13:20 +0000
> > @@ -0,0 +1,122 @@
> > +<?php
> > +
> > +class LicenseHelperTest extends PHPUnit_Framework_TestCase
> > +{
> > +
> > +  /**
> > +   * Includes test class, creates some help files for testing
> > +   */
> 
> These are comment styles I like to see (/**/ instead of //) on method names.
> // are for inline comments, imho.
> 
> Please make the narrative active instead of passive ("Include the class to
> test and create helper files to use while testing." or similar).  Finish
> sentences with punctuation :)
> 
> > +  protected function setUp()
> > +  {
> > +    require_once("../licenses/LicenseHelper.php");
> > +    symlink(__FILE__, "test_link");
> 
> This sounds a bit scary: what if somebody accidentally did some writing to the
> test file in one of the tests, without looking at this set-up method?  Is it
> really that costly to create a new temporary file instead?  I'd just use
> tempnam() to create one.  I'd also assign it to a property of the instance so
> it's easily available from tests that need to use it (and from tearDown), so
> it wouldn't have to be hard-coded in multiple places.
> 
> Also, you are using this only in a single test, yet it's created and destroyed
> with every test in this test class: that's a big waste.  Let's move it to that
> one test we do need it in, wrap it in a try..finally block (hopefully PHP has
> that, if not, in a generic exception catcher :)
> 
> > +  }
> > +
> > +  /**
> > +   * Removes help files
> 
> "Remove helper files used in testing."
> 
> > +   */
> > +  protected function tearDown()
> 
> Trailing whitespace :)
> 
> > +  {
> > +    unlink("test_link");
> > +  }
> > +
> > +  /**
> > +   * Test with directory
> 
> "Running checkFile on a directory path returns false."
> 
> > +   */
> > +  public function testCheckFile_nonFile()
> 
> Please name all tests for methods as test_methodName_something() instead of
> testMethodName_something.
> 
> > +  {
> > +    $this->assertFalse(LicenseHelper::checkFile(dirname(__FILE__)));
> > +  }
> > +
> > +  /**
> > +   * Test with link
> 
> "Running checkFile on a symbolic link to an existing file returns true."
> 
> Which reminds me, what happens if it's run on a symbolic link to a non-
> existant file?
> 

Very good point. Unfortunately, I'm not sure what was the authors expected behavior for this method (suggestion below), but here's how is_file and is_link work for links and files in PHP.

is_file(file) - TRUE
is_file(hard_link) - TRUE
is_file(soft_link) - TRUE
is_file(broken_soft_link) - FALSE

is_link(file) - FALSE
is_link(hard_link) - FALSE
is_link(soft_link) - TRUE
is_link(broken_soft_link) - TRUE

Some of those are really odd, and in my opinion we should only use the standard is_file function, because it basically serves to identify files and links from directories.

> > +   */
> > +  public function testCheckFile_link()
> > +  {
> > +    $this->assertTrue(LicenseHelper::checkFile("test_link"));
> > +  }
> > +
> > +  /**
> > +   * Test with file
> 
> "Running checkFile on a regular file returns true."
> 
> > +   */
> > +  public function testCheckFile_file()
> > +  {
> > +    $this->assertTrue(LicenseHelper::checkFile(__FILE__));
> > +  }
> > +
> > +  /**
> > +   * Get file list from a file
> 
> "getFileList throws an InvalidArgumentException when passed an argument
> pointing to a file."
> 
> > +   */
> > +  public function testGetFilesList_file()
> > +  {
> > +    try {
> > +      $file_list = LicenseHelper::getFilesList(__FILE__);
> > +      $this->assertTrue(FALSE);
> > +    } catch (InvalidArgumentException $e) {
> > +      $this->assertTrue(TRUE);
> > +    } catch (Exception $e) {
> > +      $this->assertTrue(FALSE);
> > +    }
> > +  }
> 
> Any reason you are not using the following approach here:
> 
> http://www.phpunit.de/manual/3.2/en/writing-tests-for-phpunit.html#writing-
> tests-for-phpunit.exceptions.examples.ExceptionTest.php
> 
> 
> > +
> > +  /**
> > +   * Get file list from a directory
> 
> "getFileList returns a list of filenames in that directory."
> 
> > +   */
> > +  public function testGetFilesList_dir()
> > +  {
> > +    $file_list = LicenseHelper::getFilesList(dirname(__FILE__));
> > +    $this->assertNotEmpty($file_list);
> > +    $this->assertContains(basename(__FILE__), $file_list);
> > +
> > +    // Remove '.' and '..'.
> > +    $expected_count = count(scandir(dirname(__FILE__))) - 2;
> > +    $this->assertCount($expected_count, $file_list);
> > +  }
> 
> I dislike this test:  if there was a bug in getFilesList so that it read a
> different directory than the one passed in, it might still accidentally
> succeed (if the one it worked on had the same number of files in it).  We
> should construct all our test data on-the-fly.  I don't know how's that best
> done with PHP, but in Python, I'd do the following:
> 
>  - create a temporary directory and store the path
>  - put a few files I want in
>  - assert that getFilesList() returns exactly the files I put in
>  - remove the temporary directory in a "finally" block
> 
> > +
> > +  /**
> > +   * Test with pattern which will not match any filename.
> 
> I'll let you come up with better narrative for further test cases in the style
> I suggested.
> 
> > +   */
> > +  public function testFindFileByPattern_noMatch()
> > +  {
> > +    $file_list = array("test.txt", "new_file.pdf");
> > +    $pattern = "/^abc/";
> > +    $this->assertFalse(LicenseHelper::findFileByPattern($file_list,
> $pattern));
> > +  }
> > +
> > +  /**
> > +   * Test with pattern which will match a filename.
> > +   */
> > +  public function testFindFileByPattern_match()
> > +  {
> > +    $file_list = array("test.txt", "new_file.pdf");
> > +    $pattern = "/test/";
> > +    $this->assertEquals("test.txt",
> 
> Trailing whitespace.
> 
> > +                     LicenseHelper::findFileByPattern($file_list,
> $pattern));
> 
> And bad indentation (tabs).  To span multiple lines, you can also do the
> following:
> 
>     $this->assertEquals(
>         "test.txt", LicenseHelper::findFileByPattern($file_list, $pattern));
> 
> > +  }
> > +
> > +  /**
> > +   * Test with no eula present.
> 
> "getTheme returns a generic Linaro-branded template when no EULA is present."
> 
> > +   */
> > +  public function testGetTheme_noEula()
> > +  {
> > +    $eula = "EULA.txt";
> > +    $filename = "snowball.build.tar.bz2";
> > +    $this->assertEquals("ste", LicenseHelper::getTheme($eula, $filename));
> > +    $filename = "origen.build.tar.bz2";
> > +    $this->assertEquals("samsung", LicenseHelper::getTheme($eula,
> $filename));
> > +    $filename = "build.tar.bz2";
> > +    $this->assertEquals("linaro", LicenseHelper::getTheme($eula,
> $filename));
> > +  }
> 
> This test doesn't match the description (or at least what I expected in it).
> It should also be split into at least 3 separate tests: one for snowball, one
> for origen, and another generic one.
> 
> > +
> > +  /**
> > +   * Test with eula present.
> > +   */
> > +  public function testGetTheme_eula()
> > +  {
> > +    $eula = "EULA.txt.test";
> > +    $this->assertEquals("test", LicenseHelper::getTheme($eula, ""));
> 
> I am not sure what this test demonstrates either.  Please improve the comment
> so readers don't have to go back to code to understand what's going on here.
> 
> 
> Finally, don't be stumped with the amount of suggestions I am making.  They
> are mostly cosmetic, but that's important in projects that multiple people are
> going to be touching on and maintaining over a long period of time.  It makes
> adjustments from one project to the other easier, and allows us to be more
> effective.
> 
> I am generally very happy with your work, so just keep it up.  And let's get
> this fixed up and then landed.
> 
>   review needs-fixing
> 
> Cheers,
> Danilo