Add possibility to use attributes for Context snippets #1549

fmatsos · 2024-11-28T14:40:06Z

Description

An old issue (cf. #1379) proposed to generate snippets with attributes in place of annotations.
I worked on it last days and finally came up with a working solution.

Proposal

For now, I didn't switch all annotations to attributes.
I add a new CLI option to use attributes if we want, and an associated config key also.

So you can now do:

behat -f progress --no-colors --snippets-for=FeatureContext --snippets-use-attributes for example.

or add the new config key in behat.yml to use attributes by default:

default:
  contexts:
    snippets:
      use_attributes: true

Although attributes are now widely supported, I think it's better to act in two phases: first, this PR, to implement attributes as an option. And later to invert and use attributes by default.

fmatsos · 2024-11-28T14:41:52Z

PR is in draft since I need to fix certain tests yet. But you can already review the code and let me know :)

stof · 2024-11-28T15:08:47Z

I would rather migrate to attributes directly, to avoid adding a new CLI flag and configuration setting that must then be covered by backward compatibility

fmatsos · 2024-11-28T15:18:45Z

Okay, I'm agree with that. I was very conservative on this point, I see the problem with potentials BCs after. So I can switch to attributes by default of course, and update tests accordingly.

carlos-granados · 2024-11-28T16:57:50Z

@fmatsos yes, as we commented on the issue you linked, we prefer to just move to generating attributes directly. In the next days I plan to work on updating our documentation to use attributes and just mention annotations as an option that will be deprecated and dropped in the future

carlos-granados

Many thanks for this work @fmatsos There are a few changes needed, please see my comments

features/append_snippets.feature

src/Behat/Behat/Context/Snippet/Generator/ContextSnippetGenerator.php

src/Behat/Behat/Snippet/Generator/SnippetGenerator.php

acoulton

As you will need to work on the feature files anyway to address @carlos-granados feedback, could I also please ask you to split your final commit test: update tests to check features in place of annotations into two:

A first one that only updates the features / expectations that are to do with generating snippets, where the output has changed because of the code changes in this branch.
A separate one (if you wish) to convert existing features to use attributes instead of annotations - these aren't strictly required for this PR and it would be easier to see the diff for the feature itself, and to see why the tests are now failing, if they were kept in a separate commit.

Thanks!

fmatsos · 2024-12-03T13:00:34Z

I've processed your feedbacks, and I've also made a few additional changes to add required uses for attributes. I've rollback the test updates to separate them into two commits as suggested by @acoulton.

The next commit for tests, which will replace all annotations with attributes, will come when I've solved two remaining problems. I have two scenarios that no longer work (locally):.

    features/legacy_snippets.feature:119
    features/snippets.feature:117

I'm looking why I get these errors now.

Thanks again for your review!

features/append_snippets.feature

fmatsos · 2024-12-03T14:57:02Z

The next commit for tests, which will replace all annotations with attributes, will come when I've solved two remaining problems. I have two scenarios that no longer work (locally):.
    features/legacy_snippets.feature:119
    features/snippets.feature:117

I finally found the problem. On some regex patterns, like \(\d+), backslashe before paranthesis is not escaped as required. So regex is not valid.

acoulton

This is taking shape well.

How hard would it be to output the additional use statements when displaying the "define them with these snippets" message to the user? Or to generate (or convert) snippets for display as FQCN.

If not, could we add a generic "You may also need to add use statements for the Behat\Step\(Given/When/Then) attributes" that always appears?

I'm just thinking for things like PendingException/TableNode they will get a runtime error if the class isn't imported. But by design if an attribute is undefined/not imported it will just be ignored, which could cause confusion if the user copies and pastes the snippet without realising.

src/Behat/Behat/Definition/Call/Given.php

fmatsos · 2024-12-04T14:11:30Z

How hard would it be to output the additional use statements when displaying the "define them with these snippets" message to the user? Or to generate (or convert) snippets for display as FQCN.

Tell me if I understand correctly @acoulton: you ask if it'll not be better to remove the use statements from snippets for attributes, and just display their FQCN (or generic message) to user instead?
In which case I can't give a response now but I can take a look.

carlos-granados · 2024-12-04T14:37:32Z

@fmatsos there are two cases where we use the snippets:

When they are directly added to the context class
When they are displayed and the user needs to take care of adding them to the class.

In the first case, the ideal would be that the use statements were added directly without user intervention. In the second case it would be good if we could print the use statements as part of the snippet output so that the user could also copy them

I think that what @acoulton meant was that if these two options were not possible, we should look into using the FQN in the snippets, but only if we could not add the use statements as that is a better solution.

@acoulton please confirm

acoulton · 2024-12-04T14:59:56Z

@fmatsos sorry that I wasn't clear.

Yes, exactly as @carlos-granados says, in the second case where we display snippets for the user to add I think in order of preference:

We should print the use statements (if any) as well as the snippets;
Or we should print snippets with their FQCN for the user to import when they paste into their file;
Or worst case we should just always print a fixed message to remind them that they might need to add use statements.

fmatsos · 2024-12-05T13:48:27Z

Hi!

I'm always stuck with this error:
The regex '/^do something undefined with \(\d+)$/' is invalid: preg_match(): Compilation failed: unmatched closing parenthesis at offset 34

Occurred in files:

    features/legacy_snippets.feature:119
    features/snippets.feature:117

I did some research, set-by-step debugging, and still not find how to fix it...
I don't understand why backslash before the opening bracket is not escaped. The same is escaped rightly for other tests.

carlos-granados · 2024-12-05T14:36:25Z

I'll try to debug this later today

carlos-granados · 2024-12-05T22:36:04Z

I added a fix, you need to convert the \\ that is used in the expression to represent a backslash to \\\\. This is read by php as \\ and the regular expression interpreter reads it as \. The beauty of PHP and regular expressions 😆 🤯

fmatsos · 2024-12-06T08:38:57Z

Thanks a lot @carlos-granados!

I began to be crazy about this lol
Since all the other tests passed, I didn't think at all it was the expression that needed fixing. You save me from a big headache!

acoulton · 2024-12-06T10:12:40Z

One thing I've found that can be helpful with \\ hell is to switch to a nowdoc (<<<'REGEX') even temporarily for debugging, as PHP reads them literally without parsing/escaping any \ characters

fmatsos · 2024-12-06T13:56:42Z

One thing I've found that can be helpful with \\ hell is to switch to a nowdoc (<<<'REGEX') even temporarily for debugging, as PHP reads them literally without parsing/escaping any \ characters

Nice to know this trick!

FIY I'm on your request about use statements messages. Once it's done, this PR will be okay for final review. I'll do another PR to update the rest features from annotations to attributes (I think it'll be better for review and better to have two separate PRs for that) :)

acoulton · 2024-12-06T14:00:37Z

@fmatsos perfect, that sounds like a good plan.

i18n.php

features/snippets.feature

acoulton

One tiny fix, but otherwise this is looking great thank you 🎉

i18n.php

features/snippets.feature

src/Behat/Behat/Snippet/Printer/ConsoleSnippetPrinter.php

acoulton

Brilliant, thanks @fmatsos

loic425 · 2024-12-07T10:44:34Z

Congratulations @fmatsos ;) This will be really great to use.

carlos-granados

Great job overall @fmatsos, just a few small comments

i18n.php

src/Behat/Behat/Context/Snippet/ContextSnippet.php

src/Behat/Behat/Context/Snippet/Generator/FixedContextIdentifier.php

src/Behat/Behat/Context/Snippet/Generator/FixedPatternIdentifier.php

src/Behat/Behat/Snippet/Printer/ConsoleSnippetPrinter.php

carlos-granados

All good now @fmatsos Do you think you might rebase it to the latest master? Also you may want to squash some of the commits

Signed-off-by: Franck Matsos <franck.matsos@akawaka.fr>

fmatsos · 2024-12-08T19:57:50Z

Hi @carlos-granados!
I squashed into 3 commits. The branch was also already up to date with master.
Everything should be ok now 😃

Have a good evening!

stof · 2024-12-09T10:36:05Z

src/Behat/Behat/Context/Snippet/Generator/ContextSnippetGenerator.php

+    private function preparePattern(string $pattern): string
+    {
+        $pattern = str_replace('%', '%%', $pattern);
+        $pattern = str_replace('\\\\', '\\\\\\\\', $pattern);


instead of those replacements, I suggest removing the quotes from the template and using var_export() to make PHP generate the proper literal representation of the string with any needed escaping.

You suggest something like that?

$pattern = str_replace('%', '%%', $pattern); return var_export($pattern, true);

4 tests from snippets.feature fail with this change, since some \ are now escaped too. I presume it's normal and expected.

@fmatsos I'm guessing that is what @stof meant, it looks ok to me. Could you clarify about the tests - is it that snippets.feature is currently valid and using var_export gives the wrong result? Or is it that the current expectations in snippets.feature are not correctly escaped and using var_export fixes that? Thanks

Of course! I'll test the final regex to check if new pattern is valid or not.

Hi @stof

I deeply check your suggestion, but it seems I can't use var_export in this case. If I use it, I have \ from regex keywords that are escaped too. So the pattern is not valid anymore.

Example:

expected: #[Then('/^do something undefined with \\(\d+)$/')]

result: #[Then('/^do something undefined with \\\\(\\d+)$/')]

@fmatsos @stof I've finally had a chance to look at this (I wanted to make sure it wasn't masking a bigger issue about \ escaping) and I see the problem.

A \ only needs to be escaped before certain other characters (e.g. \n would need to be \\n) - if the following character is "safe" then a single \ is treated as a literal \.

So these two step definitions parse exactly the same (I've checked) and either would be valid:

#[Then('/I should have (\d+) apple(s)/')] #[Then('/I should have (\\d+) apple(s)/')]

Historically we have generated the first version - which I think is easier for the user to read, certainly once patterns get more complex.

But var_export plays it safe and always escapes a \ rather than checking the following character to see if that is strictly necessary.

I would usually prefer var_export for safety, but in this case I favour the current implementation (not using var_export) to generate simpler strings. These are only suggestions, and they're to be applied by developers : in the very rare edge case that we suggest an improperly-escaped snippet definition, the user can / will fix it when they add it to their Context.

stof · 2024-12-09T10:36:52Z

src/Behat/Behat/Context/Snippet/Generator/ContextSnippetGenerator.php

-     * @param string $contextClass
-     *
-     * @return string[]
+     * @return array<string, array<string, string>


This type is wrong. It does not return an array of arrays as it returns the proposed methods for a single context class. the proper return type is array<string, string>

Btw, the current annotation is even totally broken because it misses a > to be a valid type.

Well spotted @stof, sorry I missed that. @fmatsos do you have time to fix that? Thanks

Yes, I already have the fix on a new branch with the next proposal ;)

Since the var_export question is resolved, I've just pushed a fix for this in #1569

Fixes a typo that slipped in with Behat#1549 as per Behat#1549 (comment)

fmatsos force-pushed the feat/add-attributes-to-snippets branch from 8b3ad91 to 86929fb Compare November 28, 2024 15:39

fmatsos force-pushed the feat/add-attributes-to-snippets branch from 86929fb to 33849c0 Compare November 29, 2024 11:18

carlos-granados requested changes Dec 1, 2024

View reviewed changes

acoulton requested changes Dec 1, 2024

View reviewed changes

fmatsos force-pushed the feat/add-attributes-to-snippets branch from 33849c0 to 26b5126 Compare December 3, 2024 12:53

fmatsos commented Dec 3, 2024

View reviewed changes

features/append_snippets.feature Show resolved Hide resolved

acoulton reviewed Dec 4, 2024

View reviewed changes

src/Behat/Behat/Definition/Call/Given.php Outdated Show resolved Hide resolved

acoulton mentioned this pull request Dec 6, 2024

chore: update docs to use Attributes instead of Annotations Behat/docs#178

Merged

fmatsos force-pushed the feat/add-attributes-to-snippets branch from 1541d9d to 8afa651 Compare December 6, 2024 15:20

fmatsos commented Dec 6, 2024

View reviewed changes

i18n.php Show resolved Hide resolved

fmatsos commented Dec 6, 2024

View reviewed changes

features/snippets.feature Show resolved Hide resolved

fmatsos marked this pull request as ready for review December 6, 2024 15:26

fmatsos requested review from acoulton and carlos-granados December 6, 2024 15:28

acoulton requested changes Dec 6, 2024

View reviewed changes

i18n.php Show resolved Hide resolved

features/snippets.feature Show resolved Hide resolved

src/Behat/Behat/Snippet/Printer/ConsoleSnippetPrinter.php Outdated Show resolved Hide resolved

src/Behat/Behat/Snippet/Printer/ConsoleSnippetPrinter.php Outdated Show resolved Hide resolved

fmatsos force-pushed the feat/add-attributes-to-snippets branch from 8afa651 to 010ba08 Compare December 6, 2024 21:44

fmatsos requested a review from acoulton December 6, 2024 21:45

acoulton approved these changes Dec 6, 2024

View reviewed changes

carlos-granados requested changes Dec 7, 2024

View reviewed changes

fmatsos requested a review from carlos-granados December 8, 2024 11:05

carlos-granados approved these changes Dec 8, 2024

View reviewed changes

fmatsos force-pushed the feat/add-attributes-to-snippets branch from 9479620 to 232d6c0 Compare December 8, 2024 19:52

fmatsos added 3 commits December 8, 2024 20:53

feat: use attributes by default for snippets

1c2fc47

Signed-off-by: Franck Matsos <franck.matsos@akawaka.fr>

test: update tests for attributes snippets

f00543e

Signed-off-by: Franck Matsos <franck.matsos@akawaka.fr>

feat: add use statements output

2fe75e8

Signed-off-by: Franck Matsos <franck.matsos@akawaka.fr>

fmatsos force-pushed the feat/add-attributes-to-snippets branch from 232d6c0 to 2fe75e8 Compare December 8, 2024 19:53

fmatsos mentioned this pull request Dec 9, 2024

Snippets using attributes need new import statements or using fully qualified names #1553

Closed

Merge branch 'master' into feat/add-attributes-to-snippets

2ac177a

acoulton merged commit a3555ca into Behat:master Dec 9, 2024
17 checks passed

stof reviewed Dec 9, 2024

View reviewed changes

acoulton mentioned this pull request Dec 9, 2024

[RFC] Add Support for PHP File Configuration #1539

Closed

fmatsos deleted the feat/add-attributes-to-snippets branch December 10, 2024 09:38

This was referenced Dec 10, 2024

Migrate features from annotations to attributes #1563

Closed

feat: migrate annotations to attributes for features #1564

Merged

carlos-granados mentioned this pull request Dec 10, 2024

Generate snippets with Attributes instead of Annotations #1379

Closed

acoulton added a commit to acoulton/Behat that referenced this pull request Dec 17, 2024

chore: Fix typo in return type annotation

9acb637

Fixes a typo that slipped in with Behat#1549 as per Behat#1549 (comment)

acoulton mentioned this pull request Dec 17, 2024

chore: Fix typo in return type annotation #1569

Merged

Uh oh!

Add possibility to use attributes for Context snippets #1549

Add possibility to use attributes for Context snippets #1549

Uh oh!

Conversation

fmatsos commented Nov 28, 2024

Description

Proposal

Uh oh!

fmatsos commented Nov 28, 2024

Uh oh!

stof commented Nov 28, 2024

Uh oh!

fmatsos commented Nov 28, 2024

Uh oh!

carlos-granados commented Nov 28, 2024

Uh oh!

carlos-granados left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

acoulton left a comment

Choose a reason for hiding this comment

Uh oh!

fmatsos commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fmatsos commented Dec 3, 2024

Uh oh!

acoulton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fmatsos commented Dec 4, 2024

Uh oh!

carlos-granados commented Dec 4, 2024

Uh oh!

acoulton commented Dec 4, 2024

Uh oh!

fmatsos commented Dec 5, 2024

Uh oh!

carlos-granados commented Dec 5, 2024

Uh oh!

carlos-granados commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmatsos commented Dec 6, 2024

Uh oh!

acoulton commented Dec 6, 2024

Uh oh!

fmatsos commented Dec 6, 2024

Uh oh!

acoulton commented Dec 6, 2024

Uh oh!

Uh oh!

Uh oh!

acoulton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

acoulton left a comment

Choose a reason for hiding this comment

Uh oh!

loic425 commented Dec 7, 2024

Uh oh!

carlos-granados left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fmatsos commented Dec 3, 2024 •

edited

Loading

carlos-granados commented Dec 5, 2024 •

edited

Loading

fmatsos Dec 10, 2024 •

edited

Loading