Skip to content

/ cpython Public

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

Jump to bottom

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

Merged
merged 2 commits into from Sep 22, 2021
Merged

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

merged 2 commits into from Sep 22, 2021
+34 −9

Conversation

@fish2000
Copy link
Contributor

@fish2000 fish2000 commented Feb 4, 2020 •
edited by bedevere-bot

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
@fish2000 fish2000 force-pushed the bpo-39549-reprlib-Repr-fillvalue branch from cf56ade to c27e567 Feb 4, 2020
@codecov
Copy link

@codecov codecov bot commented Feb 4, 2020 •
edited

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.

eamanu
eamanu reviewed Feb 5, 2020
View changes
Copy link
Contributor

@eamanu eamanu left a comment

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 Feb 6, 2020
@rhettinger rhettinger self-assigned this Feb 6, 2020
@rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Feb 6, 2020

Why not pass the fillvalue into _init_?

@fish2000
Copy link
Contributor Author

@fish2000 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?

rhettinger
rhettinger requested changes Sep 19, 2021
View changes
Copy link
Contributor

@rhettinger rhettinger left a comment

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

Include a versionchanged directive.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Sep 19, 2021

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 rhettinger force-pushed the bpo-39549-reprlib-Repr-fillvalue branch from c27e567 to 44ba119 Sep 22, 2021
@rhettinger rhettinger merged commit 8c21941 into python:main Sep 22, 2021
12 checks passed
@rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Sep 22, 2021

Thanks Zach.

@fish2000
Copy link
Contributor Author

@fish2000 fish2000 commented Sep 25, 2021

🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eamanu eamanu

@rhettinger rhettinger

Assignees

@rhettinger rhettinger

Labels
CLA signed
Projects
None yet
Milestone
No milestone
6 participants
@fish2000 @rhettinger @bedevere-bot @eamanu @ambv @the-knights-who-say-ni