Closed k00ni closed 4 years ago
Thank you for your trust. First I was hesitant because changed a lot of your initial code, but thinking about the long run changed my mind. My approach was to keep your concepts and style if possible, but implement it using more-or-less state of the art PHP. The whole PHP ecosystem changes towards more type-oriented programming, for instance. Also having a known coding style, like PSR-2, helps foreign developers to understand your code.
We can leave this PR open, in case you wanna make additional performance checks for instance.
I think it’s better to move faster and release the higher quality PHP code, so that future pull requests can be based on that. What do you think?
I wanted to see if I can fix failing tests in PHP 7.4 (#28), ended up reworked many parts to get cleaner code and a better view on what is happening.
tl;dr: hardf code is way to much JS-like and not using proper PHP-concepts. For instance, hot swapping closures during parsing. That may feel handy, but it makes error-tracing so much harder (also static analysis). Further tooling added to make development more smooth.
@pietercolpaert: its a total overhaul of your code, hope its not too much at once.
Problems / issues
shouldNotSerialize
, which is called with parameters (even though this method hasn't any) and has no implementation (ref: https://github.com/pietercolpaert/hardf/blob/master/test/TriGWriterTest.php#L552).write
with 2 parameters, but this method only has 1 ($done
parameter not used in function) -- Therefore$done
can be removed as parameter and all callers have to be adapted to not use $done anymore.end
function has no parameters, but is used in tests with 1 parameter (e.g.TriGWriterTest::shouldSerialize
).writeTriple
andwriteTripleLine
calling$done
parameter (callable or null), if set. If not set, all raised exceptions get swallowed up. As far as I can tell, all tests aroundshouldSerialize
andshouldNoSerialize
inTriGWriter
are not trustworthy, because errors are not passed from source to test. tl;dr: Thrown exceptions need to reach the caller, so it can be handled properly. But that is not the case.N3Lexer
there is a function call likecallback(null, ['line' => $this->line, 'type' => 'comment', 'value' => $comment[1], 'prefix' => '']);
. If I see that right,callback
is from PHPUnit and not available without it. Reference: https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/Assert/Functions.php#L2045 - @pietercolpaert can you approve that?Further changes:
composer.json
to 7.1 (all version below 7.2 are not maintained anymore, but 7.1 may be used still, so i leave it for now)//it('does not work with null', function () {//TODO: Util::getLiteralType.bind(null, null).should.throw('null is not a literal');
.editorconfig
file to further help with keep our coding standards (if editor supports it)What do you think?