Add optional filename and line number information to patterns

[kellegous]

We would like to be able to indicate the line number for the relevant pattern in error/warning messages in a project. This pull request adds a nullable SourceInfo property to Pattern that may be optionally via the constructor. The Parser has been updated to always populate that property when parsing via parseFile.

When using parseString, the filename defaults to an empty string ('') but can be overwritten via the $filename parameter. Having the parameter optional maintains backwards compatibility but also makes source info available on both parse methods in the parser.

[timoschinkel]

Thank you for your contribution. I will have a look at the proposed changes.

[timoschinkel]

I like the idea, but two things are preventing me from merging as is; this adds new functionality, so following semantic versioning this should create version 2.3.0.

I'm wondering about what the default value for $filename should be for Parser::parseString(). Right now you have made it a nullable string defaulting to an empty string. Looking at the updated tests this is intentional. But the default value for the (private) Parser::parseIterable() is null. I think having the line number is valuable information, independent whether you have specified a filename.

Would it make sense to change the signature to Parser::parseString(string $lines) (so no filename), and make SourceInfo::$filename nullable. That way it is possible to always have a line number in the rule.

If you can come up with a good use-case for specifying a filename when parsing from a string, then I am also okay with using the signature Parser::parseString(string $lines, ?string $filename = null).

[kellegous]

I like the idea, but two things are preventing me from merging as is; this adds new functionality, so following semantic versioning this should create version 2.3.0.

Absolutely, you're right, this adds new backwards compatible public API. Will fix.

Would it make sense to change the signature to Parser::parseString(string $lines) (so no filename), and make SourceInfo::$filename nullable. That way it is possible to always have a line number in the rule.

I was torn on how to handle Parser::parseString(string $lines) and flip-flopped on a null filename or an empty filename. I'll change SourceInfo::getFilename to return a ?string and we should be good there.

As to the use case for Parse::parseString($lines, $filename), here's what I had in mind, which is a somewhat common pattern for parsers that either take a path or raw contents (in this case a string).

$data_from_owners = file_get_contents('OWNERS');
// just an example, the idea is that you often need to do something with the contents of the file you will
// be parsing, but you still want to parse the contents as if they had been read directly from a file.
$digest = hash('sha256', $data_from_owners); 
$patterns = Parser::parseString($data_from_owners, 'OWNERS');

The idea is that there are cases where you are ultimately reading from a file but you need to have access to the data that resides in the file prior to parsing it. All of that said, though, this is not a use case I currently have. It is, indeed, speculative. As the maintainer, I'll defer to you if you think the speculative use is worth it.

Thank you for the review. I'll update this PR soon.

[kellegous]

Updated. Just let me know if you think the use case for passing $filename to Parser::parseString is worthwhile. If not, I'll remove it.

[timoschinkel]

Let's make the SourceInfo not nullable. If that's done I'll merge this one.

[timoschinkel]

Thanks for the contribution. I've merged and tagged version 2.3.0.