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

bpo-27432: added max_length param to safe_repr and wrote tests for it #13091

Closed
wants to merge 1 commit into from

Conversation

Julsy
Copy link
Contributor

@Julsy Julsy commented May 4, 2019

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@serhiy-storchaka
Copy link
Member

What is the purpose of adding a parameter to internal function if it is never used?

@gpshead
Copy link
Member

gpshead commented May 7, 2019

What is the purpose of adding a parameter to internal function if it is never used?

Julsy and I were pairing on this issue at the PyCon US mentored sprint. This is the first step in the work to clean up and clarify safe_repr usage. More details in the bug.

@gpshead
Copy link
Member

gpshead commented Jun 3, 2019

I spent a while looking at this and what it'd take to wind up with a TestCase.maxReprLength style implementation to limit the length of unittest assertion failure reprs (as described in the bpo issue). I don't see a nice path forward via safe_repr as is.

adding max_length as this PR does is easy enough, but plumbing everything through is rather gross. mostly mechanical changes I started in a client before abandoning the work:

  1. change all TestCase method safe_repr calls to self._safe_repr method calls so that the method could pass self.maxReprLength in via max_length=.
  2. realize that this is mostly pointless without also updating unittest.util._common_shorten_repr which calls safe_repr in a similar manner. do that...
  3. and notice how we have many calls of safe_repr on safe_repr'ed things and the nesting between _common_shorten_repr and safe_repr which makes for messy uninformative potentially misleading error messages when maxReprLength was actually set to a value.

Overall I think we should backup and reconsider what we want our unittest assertions to do and how if we're going to add error message length constraining as a feature. shoehorning it in via the existing repr implementations doesn't feel right.

So I'll close this PR and drop a similar note on the bug in case anyone wants to pick this up in the future. unittest.safe_repr (aka unittest.util.safe_repr) remains an undocumented API.

@gpshead
Copy link
Member

gpshead commented Jun 3, 2019

Thanks for working on this at PyCon this year Julia! Attention on issues is good and sometimes it takes trying to implement something before realizing it isn't the right thing to do. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants