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

Allows use <params> in parsers defined steps #433

Merged

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?

pytest_bdd/scenario.py Outdated Show resolved Hide resolved
pytest_bdd/parser.py Outdated Show resolved Hide resolved
@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 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

So parsers could parse values defined in Examples
@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
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.

None yet

2 participants