Skip to content

bpo-39549: reprlib.Repr uses a “fillvalue” attribute #18343

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

Merged
merged 2 commits into from
Sep 22, 2021

Conversation

fish2000
Copy link
Contributor

@fish2000 fish2000 commented Feb 4, 2020

Whereas the code for reprlib.Repr had previously used a hardcoded
string value of '...', this PR updates it to use of a “fillvalue”
attribute, whose value defaults to '...' and can be reset in either
individual reprlib.Repr instances or in subclasses thereof.

A new assert has been added to test_reprlib.py to verify the new
attributes’ functionality.

https://bugs.python.org/issue39549

@codecov
Copy link

codecov bot commented Feb 4, 2020

Codecov Report

Merging #18343 into master will decrease coverage by 1.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #18343       +/-   ##
===========================================
- Coverage   83.18%   82.11%    -1.07%     
===========================================
  Files        1570     1954      +384     
  Lines      414127   583278   +169151     
  Branches    44406    44409        +3     
===========================================
+ Hits       344505   478983   +134478     
- Misses      59981    94653    +34672     
- Partials     9641     9642        +1     
Impacted Files Coverage Δ
Lib/test/test_asyncore.py 93.62% <0.00%> (-0.18%) ⬇️
Lib/idlelib/squeezer.py 93.23% <0.00%> (ø) ⬆️
Modules/expat/xmltok_impl.c 62.10% <0.00%> (ø)
Modules/_decimal/libmpdec/difradix2.c 100.00% <0.00%> (ø)
Modules/md5module.c 94.01% <0.00%> (ø)
Modules/_tracemalloc.c 84.59% <0.00%> (ø)
Modules/unicodedata.c 89.34% <0.00%> (ø)
Parser/token.c 94.28% <0.00%> (ø)
Modules/_blake2/blake2module.c 90.90% <0.00%> (ø)
Modules/resource.c 77.51% <0.00%> (ø)
... and 380 more

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 5807efd...c27e567. Read the comment docs.

Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

Hi,

I think setting a variable could be confused in the sense that is more quickly see
the '...' than "self.fillvalue". But is a personal point of view.

Despite of this could be a good improve to if we want to avoid 'hardcoded' things.

@csabella csabella requested a review from rhettinger February 6, 2020 12:02
@rhettinger rhettinger self-assigned this Feb 6, 2020
@rhettinger
Copy link
Contributor

Why not pass the fillvalue into _init_?

@fish2000
Copy link
Contributor Author

fish2000 commented Feb 7, 2020

@rhettinger I would be happy to do that – I only set it up this way to match the existing manner of setting attributes on reprlib.Repr instances. Shall I go ahead and change it to use an argument?

Copy link
Contributor

@rhettinger rhettinger left a comment

Choose a reason for hiding this comment

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

Please update the main documentation as well: Doc/library/reprlib.rst

Include a versionchanged directive.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@rhettinger
Copy link
Contributor

Thanks Zach.

@fish2000
Copy link
Contributor Author

🎉🎉🎉

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.

6 participants