Skip to content

add ini option to disable string escape for parametrization #2830

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

Closed

Conversation

ApaDoctor
Copy link
Contributor

solving #2482
Re-created PR.

@RonnyPfannschmidt
Copy link
Member

thanks for remaking 👍 , please rebase instead of merging, im happy to give guidance if needed

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 92.567% when pulling b02c00f on ApaDoctor:disable-escape-test-id into 1480aed on pytest-dev:features.

@ApaDoctor
Copy link
Contributor Author

@RonnyPfannschmidt, it would be great.

@RonnyPfannschmidt
Copy link
Member

the majorly simplified rundown should be go to your branch, just to be safe, make a new one, then go back to the one you pushed again

now afterwards git rebase -i features should open up the interactive rebase
at that point remove the line with the merge commit and safe*exit the editor, the rebase should then run its course

after it was successfully executed you can git push -f the branch and remove the one made for safety

@ApaDoctor ApaDoctor force-pushed the disable-escape-test-id branch from b02c00f to 4547e74 Compare October 18, 2017 08:59
@ApaDoctor
Copy link
Contributor Author

ApaDoctor commented Oct 18, 2017

I did rebase:)

@RonnyPfannschmidt
Copy link
Member

you might need to update your base branches, as it seems there are conflicts (which shouldn't occur after a re-base

@nicoddemus
Copy link
Member

nicoddemus commented Oct 18, 2017

Thanks @ApaDoctor for following up.

FWIW I'm really 👎 on calling that option disable_test_id_escaping_and_forfeit_all_rights_to_community_support. I understand the point behind it, but we have to consider:

  1. Whoever first considers that flag will see the "use at your own risk" warning on the docs/command-line help/option name but will decide to use anyway, and will put that on their pytest.ini.
  2. Some time later someone else uses a problematic parametrization, and not knowing about the option, reports the error.
  3. We then point to "use at your own risk" warning in the docs/option name/command-line help, and understandably close the issue.

My point is all the above will still happen even if the name the option with that ugly name, so it won't bring us any benefits in the end. You really want to follow with that @RonnyPfannschmidt?

@RonnyPfannschmidt
Copy link
Member

an api issue cant be "documented away" - make using it ugly and very visible - i explicitly requested the very ugly name of the actual option

just remember pip - they moved everything to a _internal module and people still use it as a library

another example are the stdlib modules that are "provisional" and may get breaking api changes - that has bitten a lot of people as well

unless usage demonstrates badness at all points, an api that makes badness shouldn't be introduced because people are people

@RonnyPfannschmidt
Copy link
Member

so in order to get trough, i propose we additionally print a yellow warning that the option is enabled

@nicoddemus
Copy link
Member

I missed this discussion it seems.

unless usage demonstrates badness at all points, an api that makes badness shouldn't be introduced because people are people

I agree with you in principle, but we are talking about an option hidden in pytest.ini, not the API in the code. This will trip the later people that has to change the code, and they will still report back to us so the net result will be the same, but we will have to carry it forever.

Having said that, we can merge this as is if @ApaDoctor still has interest on this.

@RonnyPfannschmidt
Copy link
Member

@nicoddemus this option alters the api in the sense that it changes the contract on how certain values are constructed - as such its basically a option that opts in to a breaking api change

@RockBomber
Copy link
Contributor

Is there any chance to complete this PR?)

@nicoddemus
Copy link
Member

Hi @RockBomber, I'm still -0 on the whole idea, but if anybody wants to pick this up we can merge this. It needs to have the conflicts fixed and a test I think.

@nicoddemus
Copy link
Member

I'm closing this for now because nobody has demonstrated interest in picking this up, if anybody feels otherwise I think it is easier to submit a new PR anyway.

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.

5 participants