Code review comment for lp:~gesha/linaro-license-protection/build-info-support

> Nice work gesha.
> I have one general proposal: The license text should be extracted from
> the build info file and put in the separate file in the same directory
> as the build info file.

We've agreed to have a license text inside of the BUILD-INFO.txt when made discussed the format of that file.

>
> Few comments on the code also:
>
> > + public function readFile($fn)
> > + {
> > + $this->search_path = dirname($fn);
> > + $field = '';
> > + $fp_read = 0;
> > + if (is_dir($fn) or !is_file($fn) or filesize($fn) == 0) return
> false; is
> > + $file = fopen($fn, "r") or exit("Unable to open file $fn!");
>
> There are exceptions in PHP as well, I started using them in the project
> so I think it should be good idea that we stick with that.

Actually it is usual to open files such way. There is no suitable exception for that.

> > + $this->text_array[$fields[0]] = trim($fields[1]);
> > + $fp_read = 0;
> > + }
> > + $line = fgets($file);
> > + if (trim($line) == "")
> > + continue;
> > + $fields = explode(":", $line, "2");
> > + if ($fields[0] == "Files-Pattern") {
> > + $tmp_arr = array();
> > + $fp = trim($fields[1]);
> > + while(!feof($file)) {
> > + $line = fgets($file);
> > + if (trim($line) == "")
> > + continue;
> > + $fields = explode(":", $line, "2");
> > + if (in_array($fields[0], $this->multiline_vars)) {
> > + $field = $fields[0];
> > + if (isset($fields[1]))
> > + $tmp_arr[$fields[0]] = trim($fields[1]);
> > + while(!feof($file)) {
> > + $line = fgets($file);
> > + if (trim($line) == "")
> > + continue;
> > + $fields = explode(":", $line, "2");
> > + if(in_array($fields[0], $this->fields_defined))
> {
> > + if ($fields[0] == "Files-Pattern") {
> > + fseek($file, -(strlen($line)),
> SEEK_CUR);
> > + break 2;
> > + }
> > + break;
> > + }
> > + $tmp_arr[$field] = $tmp_arr[$field].
> > + "\n".rtrim($line);
> > + }
> > +
> > + }
> > + if (isset($fields[1])) {
> > + $tmp_arr[$fields[0]] = trim($fields[1]);
> > + }
> > + }
> > + $this->text_array[$fp] = $tmp_arr;
> > + unset($tmp_arr);
> > + }
> > + }
> > + fclose($file);
> > +
> > + return true;
> > + }
> > +
>
> First, the code above is not commented at all and was very hard to read.
> I think this will change anyway if you apply the license text extraction
> from file so I will not comment it further just now. Three nested
> 'while' loops certainly don't look good when you basically need to go
> through file only once.
>

> >
> > === modified file 'licenses/license.php'
> > --- licenses/license.php 2012-05-11 08:23:42 +0000
> > +++ licenses/license.php 2012-05-28 09:45:23 +0000
>
> Indentation changes makes the diff harder to read. Please keep in mind
> to do this kind of work in 2 check-in please.

There were not only indentation changes, but anyway your note is good.

>
>
> > + /**
> > + * Running readFile on a directory fnameh returns false.
> > + */
> > + public function test_readFile_nonFile()
> > + {
> > + $this->assertFalse(BuildInfo::readFile(dirname(__FILE__)));
> > + }
> > +
> > + /**
> > + * Running readFile on a nonexistent file returns false.
> > + */
> > + public function test_readFile_nonexistentFile()
> > + {
> > + $this->assertFalse(BuildInfo::readFile("nonexistent.file"));
> > + }
> > +
> > + /**
> > + * Running readFile on a regular file returns true.
> > + */
> > + public function test_readFile_file()
> > + {
> > + $bi = new BuildInfo();
> > + $this->assertTrue($bi->readFile("tests/BUILD-INFO.txt"));
> > + }
> > +
> Yes it does. But it also sets the 'text-array' variable with some very
> important data. This should be tested as well securing that array is not
> empty, the format of the array (it's multidimensional) etc.

The array can be empty. But anyway tests to check for correct values are added to verify that parsing works.

>
> > + /**
> > + * Running 'get' functions on an empty fields returns false.
> > + */
> > + public function test_getFormatVersion_empty()
> > + {
> > + $this->assertFalse($this->bi->getFormatVersion());
> > + }
> > +
> > + /**
> > + * Running 'get' functions on non-empty fields returns string value.
> > + */
> > + public function test_getFormatVersion()
> > + {
> > + $this->assertInternalType('string',
> $this->good_bi->getFormatVersion());
> > + }
>
> This is not enough. Testing that all the parsed values are not empty or
> strings will not make sure that our code is 100% working. One solution
> is to make a mock BUILD_INFO file and test the exact values you have put
> there. Or use the existing one from our trunk if it's possible.

Tests are added based on BUILD-INFO.txt