Skip to content

Allows use <params> in parsers defined steps#433

Merged
olegpidsadnyi merged 2 commits intopytest-dev:masterfrom
elchupanebrej:step_parameters_substitution
Aug 17, 2021
Merged

Allows use <params> in parsers defined steps#433
olegpidsadnyi merged 2 commits intopytest-dev:masterfrom
elchupanebrej:step_parameters_substitution

Conversation

@elchupanebrej
Copy link

@elchupanebrej elchupanebrej commented Jul 14, 2021

This covers #409 #404 #293

Blocked by #391, it has to be merged first

@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

Merging #433 (231b108) into master (c7faa9f) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
+ Coverage   96.14%   96.20%   +0.06%     
==========================================
  Files          50       50              
  Lines        1684     1715      +31     
  Branches      159      161       +2     
==========================================
+ Hits         1619     1650      +31     
  Misses         38       38              
  Partials       27       27              
Impacted Files Coverage Δ
pytest_bdd/parser.py 99.55% <100.00%> (ø)
pytest_bdd/scenario.py 92.77% <100.00%> (+0.22%) ⬆️
tests/feature/test_outline.py 100.00% <100.00%> (ø)
tests/feature/test_parametrized.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7faa9f...231b108. Read the comment docs.

@elchupanebrej elchupanebrej force-pushed the step_parameters_substitution branch 3 times, most recently from c65efda to 861a34d Compare July 14, 2021 13:59
@elchupanebrej elchupanebrej marked this pull request as ready for review July 14, 2021 14:09
@elchupanebrej elchupanebrej marked this pull request as draft August 8, 2021 09:52
@elchupanebrej elchupanebrej force-pushed the step_parameters_substitution branch 4 times, most recently from 2f83eed to 2531ca0 Compare August 8, 2021 13:17
@elchupanebrej elchupanebrej marked this pull request as ready for review August 8, 2021 13:27
@elchupanebrej
Copy link
Author

@olegpidsadnyi , please review

Copy link
Contributor

@olegpidsadnyi olegpidsadnyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Can you share the use case where the examples are mixed with the step arguments?

@elchupanebrej elchupanebrej force-pushed the step_parameters_substitution branch 4 times, most recently from d5a61fe to 8cfd432 Compare August 9, 2021 12:46
@olegpidsadnyi olegpidsadnyi requested a review from youtux August 9, 2021 16:36
olegpidsadnyi
olegpidsadnyi previously approved these changes Aug 9, 2021
Copy link
Contributor

@olegpidsadnyi olegpidsadnyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. What do you think, @youtux?

@olegpidsadnyi
Copy link
Contributor

@elchupanebrej Does it really need to be dependent on #391? I'm not sure yet about that one. Cucumber uses rule option no-unused-variables for raising errors on that. Is #391 related to your change or you can make it an independent one?

@elchupanebrej
Copy link
Author

@olegpidsadnyi I'm interested in 391 also. I'll prepare a new patch with adequate API for it and will try to not use it in this patch

@elchupanebrej elchupanebrej force-pushed the step_parameters_substitution branch from fced7d7 to 3e71c12 Compare August 10, 2021 17:14
So parsers could parse values defined in Examples
@elchupanebrej elchupanebrej force-pushed the step_parameters_substitution branch from 3e71c12 to d1ff34a Compare August 10, 2021 17:37
@elchupanebrej
Copy link
Author

@olegpidsadnyi, there is no more dependence on #391

olegpidsadnyi
olegpidsadnyi previously approved these changes Aug 11, 2021
Copy link
Contributor

@olegpidsadnyi olegpidsadnyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks okay to me.

@olegpidsadnyi
Copy link
Contributor

@elchupanebrej could you make cov happy again?

@elchupanebrej elchupanebrej force-pushed the step_parameters_substitution branch from b09f6ac to 231b108 Compare August 11, 2021 21:46
@elchupanebrej
Copy link
Author

@olegpidsadnyi done

@elchupanebrej
Copy link
Author

Just one thing I don't like about this PR: values from Examples section are converted using converters and after that serialized again to str. I would like to have a possibility to substitute Step variables directly, but this would require huge refactoring

@olegpidsadnyi
Copy link
Contributor

Sorry, I was having the second thoughts and reverted the merge. I don't really understand why do we need to substitute anything at all in the name of the step. Previously it was just a total match. Otherwise a parser match.
Substitution with the stringified fixture value is a slippery slope. Especially this introduces a lot of additional code that is probably not even needed.
What was the original problem again? Need to escape the <>?

@elchupanebrej
Copy link
Author

#293 (comment)
and whole #293

For my project I've solved this by using regular expressions parser and special converter, but I really prefer DRY way.

For this PR I check two approaches - full substitution of variables and approach presented in this PR. The second one is more flexible, but more dangerous

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 this pull request may close these issues.

2 participants