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

Consistent __repr__ and __str__ methods for all types #884

Merged
merged 14 commits into from Oct 14, 2019
Merged

Conversation

samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Oct 10, 2019

Change Summary

Consistent __repr__ and __str__ method for all models, in general:

  • __str__ returns a human readable string representing the data the instances holds. Defined as something like "what you might show to an end user if you were lazy"
  • __repr__ is roughly f'ClassName({str(self)})', except commas are used in separators to make
    eval(repr(m)) roughly equal to m

other changes:

  • __pretty__ method added to integrate with python-devtools
  • display() method on SecretStr and SecretBytes has been depreciated in favour of `str(s)
  • (virtually unused and undocumented) to_string() method on BaseModel has been depreciated in favour of str(model), devtools is a much better approach.

This is a bigger than I had hoped for during v1 betas, however users have done something seriously wrong if they're parsing the output of str(x) or repr(x) and I think this is much better than what went before.

This can't be merged until either all docs examples changed, or we setup an automatic way of generating the print output in examples (I'm going to work on this).

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

@codecov codecov bot commented Oct 10, 2019

Codecov Report

Merging #884 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #884   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          16     16           
  Lines        2716   2742   +26     
  Branches      518    521    +3     
=====================================
+ Hits         2716   2742   +26
Impacted Files Coverage Δ
pydantic/typing.py 100% <ø> (ø) ⬆️
pydantic/color.py 100% <100%> (ø) ⬆️
pydantic/types.py 100% <100%> (ø) ⬆️
pydantic/utils.py 100% <100%> (ø) ⬆️
pydantic/fields.py 100% <100%> (ø) ⬆️
pydantic/error_wrappers.py 100% <100%> (ø) ⬆️
pydantic/networks.py 100% <100%> (ø) ⬆️
pydantic/json.py 100% <100%> (ø) ⬆️
pydantic/schema.py 100% <100%> (ø) ⬆️

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 e7227db...7b40091. Read the comment docs.

@samuelcolvin
Copy link
Owner Author

@samuelcolvin samuelcolvin commented Oct 10, 2019

I've looked but can't find any official (or quasi-official) guidance on what format or data should be used in __repr__() and __str__(). If there is any, a pointer would be very helpful.

Instead I looked a some big projects like numpy and django and did something simpler and sensible.

@samuelcolvin samuelcolvin requested a review from dmontagu Oct 10, 2019
@dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Oct 10, 2019

@samuelcolvin I'll think some more about the repr vs. str question, but really quickly: is there a good reason we show when a secret str is empty? Feels like a potential risk, since in that case we show the exact value of the string.

I've been meaning to ask this for a while (and seeing it in this PR reminded me), and I can create a separate issues to discuss, but figured there might be a brief answer.

@dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Oct 10, 2019

@samuelcolvin The official docs for repr say:

For many types, this function makes an attempt to return a string that would yield an object with the same value when passed to eval(), otherwise the representation is a string enclosed in angle brackets that contains the name of the type of the object together with additional information often including the name and address of the object.

Based on this, I think it might be reasonable to drop the angle brackets in some instances (e.g., for BaseModel subclasses).

Obviously the generated string wouldn't be eval-compatible in all cases (e.g., if the field type can't generate an eval-compatible repr), but it would mean that when it was compatible, the repr would look as nice as possible.

It seems to me that for most models, this change would make the repr output eval-compatible, and I think that would help make it feel more intuitive for new users.

I think it's fine if one of the field values isn't repr-friendly, and results in having an angle-bracket value printed inside the repr; I just don't think that means we should (necessarily) avoid the cleaner form entirely.

@dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Oct 10, 2019

To make my suggestion more concrete:

Version currently proposed in this PR:

  assert repr(m) == "<Model(v=[<SubModel(name='testing' count=4)>])>"

Version I'm proposing in the above comment:

  assert repr(m) == "Model(v=[SubModel(name='testing', count=4)])"

@dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Oct 10, 2019

My suggestion would be to handle str the same way, but using str rather than repr to render field values. So (with the changes to repr for SecretStr, which I think are right since it isn't eval-compatible) it would behave like this:

from pydantic import SecretStr, BaseModel


class Model(BaseModel):
    secret: SecretStr


print(str(Model(secret='abc')))
# Model(secret=SecretStr('**********'))

print(repr(Model(secret='abc')))
# Model(secret=<SecretStr('**********')>)

@samuelcolvin
Copy link
Owner Author

@samuelcolvin samuelcolvin commented Oct 11, 2019

I agree with everything except:

My suggestion would be to handle str the same way, but using str rather than repr to render field values.

This is wrong IMHO:

  1. str(Model(str_value='foo bar')) would be "Model(str_value=foo bar)" which is at all clear - imagine a much longer string, or a sub-model with many members.
  2. This is also not how other commonly used types work, eg. str(['foo bar']) is "['foo bar']".

In other words: even though you're calling str() on the object, sub-items should use repr()

@samuelcolvin
Copy link
Owner Author

@samuelcolvin samuelcolvin commented Oct 11, 2019

@dmontagu I've made changes as per your suggestions, as well as doing some work to integration with python-devtools.

Let me know what you think.

@dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Oct 11, 2019

You were totally right about the str vs represent for arguments. 🤦‍♂️

@ashears
Copy link
Contributor

@ashears ashears commented Oct 11, 2019

Code quality improved in this PR IMO, all goodness

@dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Oct 11, 2019

Looks good to me

@ashears ashears mentioned this pull request Oct 11, 2019
4 tasks
@samuelcolvin samuelcolvin mentioned this pull request Oct 14, 2019
4 tasks
@samuelcolvin samuelcolvin merged commit c3098a3 into master Oct 14, 2019
11 checks passed
@samuelcolvin samuelcolvin deleted the cleanup-repr-str branch Oct 14, 2019
andreshndz pushed a commit to cuenca-mx/pydantic that referenced this issue Jan 17, 2020
* Consistent __repr__ and __str__ methods for all types

* add change description

* devtools integration and feedback on repr methods

* fix Color repr

* tests for truncate

* add devtools section to docs

* tests for devtools

* ValidationError inheriting from Representation

* fix imports

* tweaks

* tweak docs

* exec_examples.py integration with __repr__ changes
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

3 participants