hoaproject / Compiler

The Hoa\Compiler library.
https://hoa-project.net/
453 stars 47 forks source link

Add offset support #76

Closed SerafimArts closed 6 years ago

SerafimArts commented 6 years ago

Edit by @Hywan: Fix #70.

In addition to synax analysis (lex), there is also a semantic analysis of code. And in the case of errors of semantics, it is required to understand exactly where the error occurred. Like:

function a(int $b) {}
a(null);

// Error: Integer required in XXX file on line 2 and offset 3, but null given.
// a(null);
//   ^

The only way to get this information is the TreeNode instance.

This PR adds TreeNode::getOffset() method support for tokens:

class TreeNode {
    // Additional method
    public function getOffset(): int;
}
Hywan commented 6 years ago

Thanks for your contribution! Do you think you can add tests for that?

SerafimArts commented 6 years ago

Thanks for your contribution! Do you think you can add tests for that?

sure. The main thing is not to forget =)

SerafimArts commented 6 years ago

@Hywan I do not know how write tests for this package =\

1) Missing mockery 2) No phpunit support and no phpunit.xml file 3) No behat support 4) There is no script in the composer.json file with the launch of tests 5) And even I can not run them, because even something like travis.yml does not exists with examples = (

But... I found info inside this PR! https://github.com/hoaproject/Compiler/pull/67/commits/b675e53267a3302ea4000c751a0172596f0e92d8

But... vendor/bin/hoa test:run does not works correctly: Hoa\File\File::_open(): (2) Failed to open stream hoa://Kitab/Output/C:\Users\Serafim\Projects\Compiler\Bin\Pp.php. in C:\Users\Serafim\Projects\Compiler\vendor\hoa\file\File.php at line 208.

gg wp =)

vonglasow commented 6 years ago

@SerafimArts apparently you use windows, isn't it? I have try to launch tests on my side and it works, so I guess it's a different behavior from your platform. Maybe @thehawk970 can help us to reproduce the bug?

For your information, we use atoum for unit tests so you should be able to run ./vendor/bin/atoum -d Test You also have a documentation available.

Hywan commented 6 years ago

@vonglasow We use atoum but we don't run tests with atoum directly. See the hoa/test library.

@SerafimArts As the documentation says, tests are run with:

$ vendor/bin/hoa test:run

This command runs unit test suites, integration test suites, and documentation tests (with Kitab, our last effort to provide a much better quality and documentation)

Hywan commented 6 years ago

@SerafimArts This PR on your remote adds unit tests, https://github.com/SerafimArts/Compiler/pull/1.

SerafimArts commented 6 years ago

Apart from the .gitignore modification, I approve this patch, thanks!

Any popular frameworks do this: https://github.com/laravel/laravel/blob/master/.gitignore#L6 =)

Hywan commented 6 years ago

Who said Laravel was popular :-p?

SerafimArts commented 6 years ago

Who said Laravel was popular :-p?

GitHub says :P

But I agree that having an .idea in .gitignore is controversial.

Hywan commented 6 years ago
> Total tests duration: 80.75 seconds.
> Total tests memory usage: 76.01 Mb.
> Running duration: 84.70 seconds.

Success (24 test suites, 140/140 test cases, 0 void test case, 0 skipped test case, 320246 assertions)!

Good, gonna merge that.

Hywan commented 6 years ago

@SerafimArts Do you need a release now, or you can live without for some times?

SerafimArts commented 6 years ago

@Hywan The plans contain improvements of messages in the exceptions. But the roadmap is quite large, so it's not urgent =)

"some times" how much is this? Year I can not stand, I think :D

Hywan commented 6 years ago

I'm lagging on Hoa these times. My work takes me a lot of time, even it is really exciting. I'm more active on atoum these times (https://github.com/atoum/atoum/, https://github.com/atoum/phpunit-extension, https://github.com/Hywan/atoum-teamcity-extension/) and Kitab (https://github.com/Hywan/Kitab).

Let's pass a deal. If you need a release before Decembre and it's still not here, please ping me. We have a lot of good things in the pipe for Hoa, just need hours.

SerafimArts commented 6 years ago

@Hywan kk

SerafimArts commented 6 years ago

@Hywan I think that this pool requires improvements. Not only tokens should contain an offset, but also nodes (rules). What do you think about it?

Hywan commented 6 years ago

What do you mean by it requires nodes? Do you have an example?

SerafimArts commented 6 years ago

Now not all of the TreeNode have the offset value. I forgot the rules:

#HelloImARule: # <- offset missed
    <IM_A_TOKEN> # <- have offset

In this, I think that the https://github.com/hoaproject/Compiler/issues/70 issue should not be closed =\