Add named groups for python #316

jsa34 · 2024-12-06T08:02:44Z

🤔 What's changed?

Expression matching now returns a tuple: the value as before and an optional name if the expression has a name or the regex uses a named capture group.

The format for the Cucumber Expression when specifying name AND type is:

"There are {step_count:int} steps."

Where the part before the colon is the name of the arg, and after is the type we are currently using for Cucumber Expressions.

The return type is change is currently a breaking change, and will have obviously need to put it probably behind some feature flag, and make the default return the old, expected single value again, and the new tuple only if enabled. I have not done this yet as I wanted to check the breadth of test cases if the feature was fully enabled for the PoC before implementing. Also, not sure how best to feature flag!

To resolve #206

⚡️ What's your motivation?

Python (in particular pytest-bdd) uses other args in the step definitions, such as fixtures and reserved args for "datatable" and "docstring", so just mapping step arg values to step args in the expressions is not reliable or user-friendly. It is a blocker currently for adopting Cucumber Expressions into the pytest-bdd framework.

🏷️ What kind of change is this?

💥 Breaking change (incompatible changes to the API)

♻️ Anything particular you want feedback on?

Approach, public API changes, format of the named args, whether it's acceptable in general!

📋 Checklist:

I agree to respect and uphold the Cucumber Community Code of Conduct
I've changed the behaviour of the code
- I have added/updated tests to cover my changes.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
Users should know about my change
- I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

jsa34 · 2024-12-06T08:04:10Z

This is just my first draft where I made sure that everything "works".

If/once the approach is agreed, I will still need to do tests, documentation, etc., but I thought it more prudent to share the current approach I am suggesting before doing these things!

Feedback most welcome!

I also committed quite a bit of refactoring unrelated to this PR, which I will undo and raise separate PRs for

luke-hill · 2025-01-20T19:06:45Z

The last time we've done a big change like this I think was the abolition of the Transformer proc. And the introduction of this (cucumber-expressions library), "proper" (But I could be wrong here). So I'd prefer that we go down the route of releasing this all simultaneously.

In terms of feature flags, I'm happy for it to sit behind feature flags - but I'd rather it be simple and purely from a dev POV to avoid us needing to review a leviathan PR. In other words, by all means I'm happy for it to be technically easier to work in whatever way is best, but come our next full release I'd prefer it to be entirely enabled (The major to include this does not need to be the next one - currently v19)

I'm only one of the main contributors though, so it's not entirely my decision. But when cucumber-expressions were released, they were released simultaneously - Admittedly this was a long time ago with many more different people at the helm.

TL;DR - I'm pro this change, and anti it "sitting behind feature flags when released" (But during development go for it).

davidjgoss · 2025-01-20T21:16:19Z

I don’t have a particular view on the change itself, but do I agree with @luke-hill re flags - I don’t see a lot of value in flags here vs just making it a semver major change and calling attention to it in the release notes. Cucumber implementations that use this library will be pinned to at least a minor range, and other consumers should similarly assume breaking changes in a major.

mpkorstanje

Mostly looks good to me, but I do have some remarks, nothing major though. Apologies if they're a bit scattered, I'm throwing these out as I go through the PR.

Also please do add an entry to the CHANGELOG.

mpkorstanje · 2025-01-23T13:52:15Z

python/cucumber_expressions/argument.py

+        for item in parameter_types_and_names:
+            if not isinstance(item, tuple) or len(item) != 2:
+                raise CucumberExpressionError(
+                    f"Expected a tuple of (ParameterType, Optional[str]), but got {type(item)}: {item}"


This error is very technical. What should a user do if/when they encounter this error?

Also do most users know what a tuple is?

mpkorstanje · 2025-01-23T13:54:27Z

python/cucumber_expressions/expression.py

+    ) -> Tuple[Optional[str], Optional[ParameterType]]:
+        """Helper function to parse the parameter name and return group_name and parameter_type."""
+        if ":" in name:
+            group_name, parameter_type_name = [part.strip() for part in name.split(":")]


This allows for the empty group name, which is distinct from the None group name. Probably now what we want.

It might also be worth while to push this into the parser.

mpkorstanje · 2025-01-23T13:59:12Z

python/cucumber_expressions/expression_factory.py

+    def _extract_text_in_curly_brackets(string: str) -> list:
+        return CURLY_BRACKET_PATTERN.findall(string)
+
+    def is_cucumber_expression(self, expression_string: str):


I don't think this check is simple enough. The primary constraint is explaining to people what is and is not a cucumber expression. For Java I eventually settled on requiring that all regular expressions start with ^ or end with $ and that everything else is a Cucumber expression. This is both simple and unambiguous.

This helps avoid a situation where a user makes a mistake in a Cucumber expression, causing Cucumber to think it is a regular expressions and then fail because the regular expression also isn't valid and results in a very cryptic error message.

Is it worth standardising this check then across all flavours? I have no idea what we do in ruby as I've not dug into this stuff since the initial release some 4/5 years ago

mpkorstanje · 2025-01-23T14:02:18Z

python/cucumber_expressions/tree_regexp.py

        if source[index + 1] != "?":
-            # (X)


Please put these comments back. It's really helpful to have a reference here.

mpkorstanje · 2025-01-23T14:08:43Z

python/cucumber_expressions/tree_regexp.py

-            # (?>X)
-            return True
-        # (?<=X) or (?<!X) else (?<name>X)
-        return source[index + 3] in ["=", "!"]


For consistency between implementations it would be good to keep this similar too. It really helps if all the implementations are similar enough that you can reference a language implementation you do know.

Though the syntax for a named group is Python-specific so it would be good to add a separate case for that and comment on it.

mpkorstanje · 2025-01-23T14:21:49Z

python/cucumber_expressions/regular_expression.py


 from cucumber_expressions.argument import Argument
 from cucumber_expressions.parameter_type import ParameterType
 from cucumber_expressions.parameter_type_registry import ParameterTypeRegistry
 from cucumber_expressions.tree_regexp import TreeRegexp

+NAMED_CAPTURE_GROUP_REGEX = re.compile(r"\?P<([^>]+)>")


This should probably be non-greedy.

mpkorstanje · 2025-01-23T14:28:12Z

♻️ Anything particular you want feedback on?
Approach, public API changes, format of the named args, whether it's acceptable in general!

I wasn't aware that named capture groups in Python differ from other languages. Where Java, Javascript and Ruby us (?<name>.*) Go and Python use (?P<name>.*). It would be good to have that clear in the code.

I'm also missing some error handling around the : character, which is now a reserved character for parameter names.

And adding to the shared test set, even if failing would be good too.

luke-hill · 2025-01-23T14:31:33Z

I have one request so far (Pre-review).

Should we have a major release where we change cucumber-expressions to ban the : character to at least make the upgrade path a little less restrictive. That to me feels like a nice "small" major release we could do (I'm happy to do the work on this).

Obviously this only holds if the agreed path for naming is as specified here - which I think most of us are happy with

mpkorstanje · 2025-01-23T14:35:40Z

I'm not sure about the release strategy yet. I don't quite have time to sponsor a Java implementation, I'm currently working using the message format everywhere and technical debt that is pulling to the surface.

It does make me favor feature toggles though.

luke-hill

One thing I wanted to ask is whether you'd want to get both named for param types and regex out simultaneously or whether you'd want / consider doing them separately.

Purely thinking about the polyglot implementation (Unless you're volunteering to write a bunch of other flavours?)

luke-hill · 2025-01-25T02:57:14Z

python/cucumber_expressions/argument.py

+        for item in parameter_types_and_names:
+            if not isinstance(item, tuple) or len(item) != 2:
+                raise CucumberExpressionError(
+                    f"Expected a tuple of (ParameterType, Optional[str]), but got {type(item)}: {item}"


Also do most users know what a tuple is?

luke-hill · 2025-01-25T02:58:44Z

python/cucumber_expressions/argument.py

-        tree_regexp: TreeRegexp, text: str, parameter_types: List
+        tree_regexp: TreeRegexp,
+        text: str,
+        parameter_types_and_names: List[Tuple[ParameterType, Optional[str]]],


should this maybe be parameter_types_with_names which then would make sense because the name could often be nil (Which feels "right")

luke-hill · 2025-01-25T02:59:29Z

python/cucumber_expressions/argument.py

            raise CucumberExpressionError(
-                f"Group has {len(arg_groups)} capture groups, but there were {len(parameter_types)} parameter types"
+                f"Group has {len(arg_groups)} capture groups, but there were {param_count} parameter types/names"


think the ending of this error shouldn't be amended - the issue is still that there were an incorrect number of parameter types (The names being present / not is irrelevant for the length issue)

luke-hill · 2025-01-25T03:02:24Z

python/cucumber_expressions/expression_factory.py

+    def _extract_text_in_curly_brackets(string: str) -> list:
+        return CURLY_BRACKET_PATTERN.findall(string)
+
+    def is_cucumber_expression(self, expression_string: str):


Is it worth standardising this check then across all flavours? I have no idea what we do in ruby as I've not dug into this stuff since the initial release some 4/5 years ago

luke-hill · 2025-01-25T03:07:16Z

python/cucumber_expressions/tree_regexp.py

+        """
+        group_name_start = index + 3
+        group_name_end = source.find(">", group_name_start)
+        return source[group_name_start:group_name_end]


My python is basically non existent, but above we're using a : b and here we're using a:b - Do they mean diff things if not maybe keep things standard?

luke-hill · 2025-01-25T03:08:28Z

python/cucumber_expressions/tree_regexp.py

-    def group_builder(self):
-        return self._group_builder
+        # If it's a named group (e.g., (?P<name>...)), it's still a capturing group
+        if source[index + 2] == "P" and source[index + 3] == "<":


Earlier we use a substring over a range and here we're using 2 diff substring char matches.

In my head we should probably be using a range in all situations

luke-hill · 2025-01-25T03:10:17Z

python/tests/test_expression.py

+
+    def test_documents_match_arguments_with_names_and_spaces(self):
+        values = match(
+            "I have {  cuke_count : int  } cuke(s) and {gherkin_count: int} gherkin(s)",


Is this a python specific interpretation. I'm 90% sure we don't permit spaced out arguments inside the braces. but again I'd need to triple check

First commit: add named groups for python

abdb16a

jsa34 added 5 commits December 6, 2024 08:22

Fix flake8

f9a7afb

Remove removing support for py 3.8

ea7cef3

Remove removing support for py 3.8

0efab69

Remove removing support for py 3.8

51a3c34

Fix type hint for Generator

8272b12

mpkorstanje requested review from kieran-ryan and mpkorstanje December 6, 2024 14:44

Implement trimming whitespace for named args in cucumber expression

a51f9de

kieran-ryan added the ⚡ enhancement Request for new functionality label Dec 31, 2024

mpkorstanje marked this pull request as ready for review January 17, 2025 18:31

luke-hill self-requested a review January 20, 2025 19:06

mpkorstanje requested changes Jan 23, 2025

View reviewed changes

mpkorstanje marked this pull request as draft January 23, 2025 13:49

mpkorstanje reviewed Jan 23, 2025

View reviewed changes

luke-hill reviewed Jan 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add named groups for python #316

Add named groups for python #316

jsa34 commented Dec 6, 2024 •

edited

Loading

jsa34 commented Dec 6, 2024 •

edited

Loading

luke-hill commented Jan 20, 2025 •

edited

Loading

davidjgoss commented Jan 20, 2025

mpkorstanje left a comment •

edited

Loading

mpkorstanje Jan 23, 2025

luke-hill Jan 25, 2025

mpkorstanje Jan 23, 2025

mpkorstanje Jan 23, 2025

mpkorstanje Jan 23, 2025 •

edited

Loading

luke-hill Jan 25, 2025

mpkorstanje Jan 23, 2025

mpkorstanje Jan 23, 2025

mpkorstanje Jan 23, 2025

mpkorstanje commented Jan 23, 2025 •

edited

Loading

luke-hill commented Jan 23, 2025

mpkorstanje commented Jan 23, 2025

luke-hill left a comment

luke-hill Jan 25, 2025

luke-hill Jan 25, 2025

luke-hill Jan 25, 2025

luke-hill Jan 25, 2025

luke-hill Jan 25, 2025

luke-hill Jan 25, 2025

luke-hill Jan 25, 2025

Add named groups for python #316

Are you sure you want to change the base?

Add named groups for python #316

Conversation

jsa34 commented Dec 6, 2024 • edited Loading

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

📋 Checklist:

jsa34 commented Dec 6, 2024 • edited Loading

luke-hill commented Jan 20, 2025 • edited Loading

davidjgoss commented Jan 20, 2025

mpkorstanje left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpkorstanje Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpkorstanje commented Jan 23, 2025 • edited Loading

luke-hill commented Jan 23, 2025

mpkorstanje commented Jan 23, 2025

luke-hill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsa34 commented Dec 6, 2024 •

edited

Loading

jsa34 commented Dec 6, 2024 •

edited

Loading

luke-hill commented Jan 20, 2025 •

edited

Loading

mpkorstanje left a comment •

edited

Loading

mpkorstanje Jan 23, 2025 •

edited

Loading

mpkorstanje commented Jan 23, 2025 •

edited

Loading