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

Issue 7126 - "saferepr" to avoid BytesWarning when using --setup-show #7205

Merged
merged 8 commits into from
May 14, 2020

Conversation

lancelote
Copy link
Contributor

@lancelote lancelote commented May 9, 2020

Fix for #7126

I added a special case for bytes only as saferepr adds quotes to pure string literals. A bunch of tests depend on the original behavior, so I was not comfortable changing it globally. Scrapping saferepr output to match the original behavior doesn't sound as a trouble-proof solution too. Please correct me if it was a wrong idea.

Thank you for your time reviewing the PR 🙏

bytes parametrize parameters cause error when --setup-show is used
and Python is called with -bb flag
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @lancelote. I left some comments.

changelog/7126.bugfix.rst Outdated Show resolved Hide resolved
src/_pytest/setuponly.py Outdated Show resolved Hide resolved
changelog/7126.bugfix.rst Outdated Show resolved Hide resolved
testing/test_setuponly.py Outdated Show resolved Hide resolved
@lancelote lancelote marked this pull request as draft May 10, 2020 09:15
@lancelote
Copy link
Contributor Author

lancelote commented May 10, 2020

GitLab is down so linting can't succeed, unfortunately.

Changes after the review:

  • Reword changelog entry to hide internal details
  • Rename test to better suit the actual fix
  • Reference issue ID in the test
  • Unconditionally use saferepr for all types
  • Adjust setuponly tests to match the previous change

@bluetech Thank you for the valuable feedback 🙇🏻‍♂️ Please feel free to point me to any other improvements I can add here.

@lancelote lancelote marked this pull request as ready for review May 10, 2020 10:34
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM, just a couple nits.

I'll leave this open for a few days to let other chime in if they have an opinion about the change to always use repr (I think it makes sense and even in the str case makes it clearer), then I'll merge.

changelog/7126.bugfix.rst Outdated Show resolved Hide resolved
testing/test_setuponly.py Outdated Show resolved Hide resolved
lancelote and others added 2 commits May 10, 2020 16:58
Co-authored-by: Ran Benita <ran@unusedvar.com>
Co-authored-by: Ran Benita <ran@unusedvar.com>
@Zac-HD Zac-HD added topic: reporting related to terminal output and user-facing messages and errors type: bug problem that needs to be addressed labels May 13, 2020
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Looks good to me - let's merge!

Thanks, @lancelote 😁

@bluetech bluetech merged commit 2ac28f6 into pytest-dev:master May 14, 2020
@The-Compiler
Copy link
Member

Thanks everyone for the fix! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: reporting related to terminal output and user-facing messages and errors type: bug problem that needs to be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants