Multiple example tables and tags support #117#119
Multiple example tables and tags support #117#119stukalin wants to merge 3 commits intoBehat:masterfrom
Conversation
stukalin
force-pushed
the
feature/issue-117
branch
2 times, most recently
from
cb0290d to
256c973
Compare
.gitignore
Outdated
| vendor | ||
| composer.phar | ||
| composer.lock | ||
| .idea/* |
There was a problem hiding this comment.
This should be in a global gitignore, not project specific (otherwise we would need eclipse, netbeans, and whatever other ides / editors, which is quite long and not useful)
There was a problem hiding this comment.
stof
left a comment
stof
left a comment
There was a problem hiding this comment.
I will stop my review here until you revert the method reordering. It is just impossible to find out what really changed.
.gitignore
Outdated
| vendor | ||
| composer.phar | ||
| composer.lock | ||
| .idea/* |
There was a problem hiding this comment.
| * | ||
| * @return Boolean | ||
| */ | ||
| public function isScenarioMatch(ScenarioInterface $scenario) |
There was a problem hiding this comment.
please don't move method around. It makes the review harder (I know need to check manually if the moved code is the same) and breaks git blame without any benefit
| * | ||
| * @return Boolean | ||
| */ | ||
| public function isScenarioMatch(ScenarioInterface $scenario) |
stukalin
force-pushed
the
feature/issue-117
branch
from
256c973 to
10468a5
Compare
|
Just a few words on what was changed. So the idea was to allow having more than 1 examples table in an outline and those examples tables can be tagged. The main changes hence were:
|
|
@stof Could you possibly proceed with the code review? That'd be just perfect:) |
|
I won't have the time today, so it will have to wait until after the Christmas weekend (I won't do open-source code review on Christmas Eve) |
|
There are a lot of comments and nesting in this code that I'd rather be private methods of the class. |
|
@everzet I refactored a bit there... |
everzet
left a comment
everzet
left a comment
There was a problem hiding this comment.
Great job overall!!!
Sadly I can't merge it yet due to some minor and one major gripe. The most critical thing here is a BC break in OutlineNode. I'd rather avoid it.
| array $tags, | ||
| array $steps, | ||
| ExampleTableNode $table, | ||
| array $tables, |
There was a problem hiding this comment.
This is a backwards compatibility break. Unless you are willing to wait till next Gherkin major, which might never happen on this codebase, you'd better approach this differently.
| array('action', 'outcome'), | ||
| array('act#3', 'out#3'), | ||
| ), $exampleTableNodes[0]->getRows()); | ||
| $this->assertEquals(array('etag2'), $exampleTableNodes[0]->getTags()); |
There was a problem hiding this comment.
This test would benefit from breaking down.
| /** @noinspection PhpUndefinedMethodInspection */ | ||
| $this->assertEquals(array($exampleTableNode2), $scenarioInterfaces[0]->getExampleTables()); | ||
| /** @noinspection PhpUndefinedMethodInspection */ | ||
| $this->assertEquals(array($exampleTableNode3), $scenarioInterfaces[1]->getExampleTables()); |
| $this->assertFalse($tagFilter->isScenarioMatch($feature, $scenario)); | ||
|
|
||
| $tagFilter = new TagFilter('@etag1&&@etag3'); | ||
| $this->assertFalse($tagFilter->isScenarioMatch($feature, $scenario), "Tags from different examples tables"); |
| - { keyword_type: 'Given', type: '前提', text: '50 を入力', line: 8 } | ||
| - { keyword_type: 'Given', type: 'かつ', text: '70 を入力', line: 9 } | ||
| - { keyword_type: 'When', type: 'もし', text: 'add ボタンを押した', line: 10 } | ||
| - { keyword_type: 'Then', type: 'ならば', text: '結果は 120 を表示', line: 11 } |
There was a problem hiding this comment.
I'd rather not touch existing etalons and add a new one instead.
| @examples_tag3 @examples_tag4 | ||
| Examples: | ||
| | state | | ||
| | missing | |
There was a problem hiding this comment.
Same here, I'd rather us not touching existing tests. You're adding new feature, so the old behaviour and tests proving it must stay unchanged.
|
I wish people's enthusiasm towards their contributions had direct impact on maintainers life and well-being. Until that happens I'm afraid maintainers will need to take care of their personal and professional lives themselves, deprioritising everything else ;) |
|
Great job overall, but I'm closing this as some important changes I suggested weren't addressed. If anyone shows interest in taking over this PR and continuing - feel free to open new PR. |
4 participants
See issue #117 for additional information. I tried to test it properly and not break anything:)