@acoulton your points are most fine to me except this:

• Deprecate getTitle() only on ExampleNode but only with a docblock tag pointing to getName() or getExampleText(), not a runtime deprecation.
• Open an issue for a future major version to consider changing the interfaces so that anything "scenario-like" has a getName() instead of a getTitle().

Why wait? If we do that later, we won't have a chance to deprecate it - it will suddenly be renamed from getTitle to getName (unless we have to wait for 2 major versions). I would define getName in both scenario and example nodes.

Additionally, I would consider creating a new NamedNodeInterface or similar that defines getName and apply it to the current two classes - since both classes would have a getName, even if someone extends those classes, nothing would break. Later (in the major version bumpp) we could merge NamedNodeInterface into some existing interface or whatever. I don't think having an interface is too important though, end users could always use union types with PHPDoc.

The point is that it would nice (for users) to code against $scenarioOrExample->getName() instead of a lot of conditions - after the major version bump that part would remain unchanged.

Another interesting thing to note: I was considering deprecating getTitle at the interface level (it is defined in KeywordNodeInterface, but now I realised this is used by a lot more nodes. I'm wondering if we should implement getName for all of them already. 🤔
PS: I also noticed that there's an OutlineNode, (of course), what should we do about getTitle there?
PSS: I get that we might want to have a PR with the least amount of changes necessary, but the situation is already pretty complex and I'd like to avoid questions and potential reverts coming up in the next major version. I think we should be mindful about that too (e.g. that this change is well documented).

@uuf6429 my reluctance to introduce new interfaces/deprecate things at interface level at this stage is exactly because I think it needs more thought about what that structure should be.

ISTR there are nodes currently defining getTitle where this is probably correct.

I think part of the problem is methods have been defined at the wrong level of the hierarchy because they appear to be similar. For example KeywordNode - most lines in the gherkin file have a keyword and some following text, but it doesn't follow that the meaning of that text is the same for all types of node - as we're finding now with ExampleNode where the text is actually not the title at all.

You can see from the existing Behat formatter implementations that actually it very rarely makes sense to call getTitle() on a KeywordNode without knowing exactly what type of node it is. That probably means the method is in the wrong place, or called the wrong thing.

But to deprecate it on the interface now, we need to find a well-designed and BC way to replace it across all the nodes that implement it : even though some nodes might actually be fine with getTitle, just through a different (or no) interface.

Really for this usecase what we want to capture is that Example is just a special kind of Scenario (possibly the only one). And they both have a name which is some text with a specific meaning & purpose.

So if we had to introduce an interface now I would call it NamedScenarioInterface and apply it only to ExampleNode and ScenarioNode.

I wouldn't use NamedNodeInterface because as with the existing interfaces, that implies nodes are similar because they happen to have a getName() method when we can see that isn't really true.

Oh and on this specific:

Why wait? If we do that later, we won't have a chance to deprecate it - it will suddenly be renamed from getTitle to getName (unless we have to wait for 2 major versions).

I don't see us going immediately from this to a new major. Indeed with current resources a new major could still be a way off, especially if we're going to look at switching parser/moving to Pickles etc.

And even when we release a major there will be a period where we are maintaining both version series.

So there will still be an opportunity to deprecate methods between now and then - I just think we should minimise that until we can give users a clear overall explanation of what will be changing.

Deprecating the method on ExampleNode where it is currently ambiguous feels fine to do right now as there is an identified problem with that specific object and we have an easy solution.

@acoulton I agree with all points.

So if we had to introduce an interface now I would call it NamedScenarioInterface and apply it only to ExampleNode and ScenarioNode.

That makes sense to me, especially since most probably we'd want consistency over those two classes. What do you think about OutlineNode though? Are we going for Scenario or ScenarioLike?

Anyway, I'll implement this interface and open this PR for review, I think we're in a good state now for that.

I think Outline is a higher level grouping of scenarios, like a feature or a suite (or a Rule if/when we support them).

It's not the same as a Scenario/Example which represents a specific set of executable steps.

So I would leave Outline alone, for the moment at least.

@acoulton side-note: there is a \Behat\Gherkin\Filter\NameFilter class which filters by title (despite the class name). 😅

@acoulton while adjusting tests, I found out that outline represents the "scenario outline" node and that, for example, iterating over scenarios in a feature file will actually yield instance of that object. Apparently it is up to caller to iterate over the "example [child] nodes" of that object while ignoring the rest of the object.

In short, a naive foreach ($scenarios as $scenario) echo $scenario->getName() would not work (although I'm not entirely sure it would make sense to support it). Because of that, I think I would implement getName()+NamedScenarioInterface on the outline class too (if we want to make it convenient). Worst case is that flattening the scenarios in the future would make that class (and its getName) disappear.

@acoulton @uuf6429 what is needed to move this forward? Can I help in any way?

Hi @carlos-granados!

I guess @acoulton might be a bit occupied at the moment, so maybe you could give the task a look? A second (well, third) set of eyes wouldn't hurt. 😄

Apart from my comments about a couple of comments that need to be updated, everything else looks OK to me

@acoulton now that his is finally merged, should we create a release so that this can be used on the Behat side?

@uuf6429 released now, thank you!

First of all, thanks again for the effort.

One quick point, can we somehow make sure to squash PRs? It's just that I'm not typically working on branches with the understanding that the commits (and more importantly, their messages) will show up verbatim in the release branch.

@uuf6429 we can certainly try, the challenge is if there are unrelated changes - for example the fixes for the file cache tests on windows in this PR - in the same branch. I don't think they should be just squashed into a single merge commit for the overall feature, and there's no way to partially squash on GitHub.

I think it's probably better if PR authors squash and force-push as appropriate before the final code review (apart from simple changes / ones that can easily be squash-merged).

But you're right that we should probably be looking at the commits & messages as well as the diff when reviewing PRs.