Skip to content

Conversation

@arhadthedev
Copy link
Member

  1. The moved code measures performance so it should belong here.
  2. The relocation lifts off a question whether python -m pprint should have tests or not.

After merging, python/cpython#92546 needs to be closed.

@arhadthedev
Copy link
MemberAuthor

This creative PR is a receiving half of python/cpython#94613.

@ericsnowcurrentlyericsnowcurrently changed the title Move a benchmark from python -m pprintAdd a benchmark based on python -m pprintJul 7, 2022
Copy link
Member

@ericsnowcurrentlyericsnowcurrently 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 working on this, Oleg! I've left some comments for you to consider.

@arhadthedev
Copy link
MemberAuthor

@ericsnowcurrently Thank you for your thorough feedback, I've addressed everything.

In addition, I've also moved a class docstring into a file comment, and reworded it together with authorship. I hope I've made it right.

Also I've added hasattr-based conditional running of _safe_repr to not fail CI for pre-3.10 CPython.

What did you have in mind relative to stdlib benchmarks? Note that we already have a bunch of micro-benchmarks for various parts of the stdlib, in distinct benchmarks.

Initially I've thought that a benchmark should be big, like Lib/test/*.py test files.

Copy link
Member

@ericsnowcurrentlyericsnowcurrently 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 the updates.

@ericsnowcurrentlyericsnowcurrently merged commit 684eafe into python:mainJul 7, 2022
@arhadthedevarhadthedev deleted the pprint branch July 8, 2022 03:12
miss-islington pushed a commit to python/cpython that referenced this pull request Jul 25, 2022
This PR couples with python/pyperformance#222 and supersedes #92560. Inspired by #93096 (comment). Automerge-Triggered-By: GH:ericsnowcurrently
@donbarbosdonbarbos mentioned this pull request Mar 12, 2025
Sign up for freeto 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.

2 participants

@arhadthedev@ericsnowcurrently