Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snippet generator - don't enforce whitespace rule for numerical parameters #844

Closed
sebrose opened this issue Jan 6, 2020 · 14 comments · Fixed by #1108
Closed

Snippet generator - don't enforce whitespace rule for numerical parameters #844

sebrose opened this issue Jan 6, 2020 · 14 comments · Fixed by #1108
Assignees

Comments

@sebrose
Copy link
Member

sebrose commented Jan 6, 2020

Summary

#657 has changed the behaviour of the snippet generator such that it no longer behaves as expected with numerical parameters.

Expected Behavior

"Given Sean is 15m from Lucy" -> "Sean is {int}m from Lucy"
"Given a coffee costs $2.50" -> "a coffee costs ${double}" - or maybe {float}, or historically {int}.{int}

Current Behavior

"Given Sean is 15m from Lucy" -> "Sean is 15m from Lucy"
"Given a coffee costs $2.50" -> "a coffee costs $2.{int}"

Possible Solution

Numerical parameters should be bounded by whitespace at one side at least.

  • $1.50 -> ${double} or ${int}.{int}
  • 15m -> {int}m
  • 500ms -> {int}ms
  • i18n -> i18n
  • $15m -> $15m

Careful consideration of whether punctuation counts as whitespace is needed. "." and "," are used differently by locale.

Context & Motivation

This change results in non-intuitive Cucumber expressions in snippets.

Your Environment

@luke-hill
Copy link
Contributor

Good spot. I think the possible solution also makes a lot of sense. And is reasonably easy to interpret (Albeit it might be a bit cumbersome to check the 2 permutations, but don't know yet).

For now a custom one with a letter (Assuming you mean time / distance), would work nicely so \d+\w for example.

@sebrose
Copy link
Member Author

sebrose commented Jan 7, 2020

Wouldn't that also match "C3PO"?

\s\d+\w+ would force the integer to be preceded by whitespace.
^\d+\w+ would catch an integer at the start of a line

@luke-hill
Copy link
Contributor

I assumed it was only one letter, so \w would be fine, or you could use a range [] to pass in all valid chars.

It's something I'd happily work on. I know Rien did most of the work alongside aslak, so I might wait to see if they're better placed. But it's quite comprehensively explained by yourself.

@sebrose
Copy link
Member Author

sebrose commented Jan 7, 2020

I don't think we should assume a single letter. Consider milliseconds, for example: ms

@aslakhellesoy
Copy link
Contributor

We should complete #771 before tackling this.

@stale
Copy link

stale bot commented Mar 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Mar 8, 2020
@mpkorstanje mpkorstanje added the 🧷 pinned Tells Stalebot not to close this issue label Mar 8, 2020
@stale stale bot removed the ⌛ stale Will soon be closed by stalebot unless there is activity label Mar 8, 2020
@mpkorstanje
Copy link
Contributor

This isn't blocked by #771. The generation of expressions is a different component.

@mpkorstanje mpkorstanje self-assigned this Jul 2, 2020
mpkorstanje added a commit that referenced this issue Jul 9, 2020
…side

Numerical parameters should be bounded by whitespace at one side at least.
```
$1.50 -> ${double}
15m -> {int}m
500ms -> {int}ms
i18n -> i18n
$15m -> $15m
```
Fixes: #844
@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jul 9, 2020

Numerical parameters should be bounded by whitespace at one side at least.

@sebrose referencing a few style guides I found that units are not always supposed to be attached to the number.

Only for angles, percentages and some currencies (USD, but not the Euro) the symbol is attached to the number. So I think all parameters should be bounded on both sides by either whitespace, punctuation, or symbols.

So good examples would be:

"Given Sean is 15 m from Lucy" -> "Sean is {int} m from Lucy"
"Given a coffee costs $2.50" -> "a coffee costs ${double}"

mpkorstanje added a commit that referenced this issue Jul 9, 2020
Numerical parameters should be bounded by whitespace, punctuation or symbols
on both sides.

```
$1.50 -> ${double}
15° Celsius -> {int}° Celcius
i18n -> i18n
$15m -> $15m
```
Fixes: #844
@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jul 9, 2020

And writing that in a mono-spaced font makes me think that the style guide might not be applicable here. 😆

@mpkorstanje
Copy link
Contributor

A good counter to only requiring a word boundary on one side would be:

Given I take flight KLM404 --> I take flight KLM404

Rather then:

Given I take flight KLM404 --> I take flight KLM{int}

@mpkorstanje
Copy link
Contributor

I'm going to err on the side of caution for now.

We've got the currency use case covered. Just not the measurements use case.

@marnen
Copy link

marnen commented Jul 9, 2020

My take: “KLM404” isn’t an integer; it’s a different thing with its own syntax (just as a phone number is). It should probably not use an {int} parameter type.

OTOH, my experience has been that declaring a custom parameter type is harder than I wish it were (which is why I still write most of my steps with regexes). Perhaps that’s where to focus effort (or perhaps I’m behind the times; I’m not programming full-time these days).

mpkorstanje added a commit that referenced this issue Jul 10, 2020
…snippets (#1108)

Numerical parameters should be bounded by whitespace, punctuation or symbols
on both sides.

```
$1.50 -> ${double}
15° Celsius -> {int}° Celcius
i18n -> i18n
$15m -> $15m
```
Fixes: #844
@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jul 10, 2020

So I've merged #1108 because its an improvement already.

@marnen perhaps you could check a few of your projects for the [\d.,]+[^\d]+ and [^\d]+[\d.,]+ patterns and see what typical examples might be?

I've checked a few projects myself and with the exception of currency and product codes the pattern simply did not occur. But this is of-course highly domain specific. The only unit of measure I could find was written out in full (e.g. "10 seconds, rather then 10s).

@mpkorstanje mpkorstanje removed the 🧷 pinned Tells Stalebot not to close this issue label Jul 10, 2020
@marnen
Copy link

marnen commented Jul 10, 2020

@mpkorstanje I use integers moderately frequently in my step definitions. I’ll find some examples and post links here.

aslakhellesoy pushed a commit to cucumber/cucumber-expressions that referenced this issue Sep 20, 2021
…snippets (#1108)

Numerical parameters should be bounded by whitespace, punctuation or symbols
on both sides.

```
$1.50 -> ${double}
15° Celsius -> {int}° Celcius
i18n -> i18n
$15m -> $15m
```
Fixes: cucumber/common#844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants