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

Replace unchanging args with one config arg within pretty-format #4076

Merged
merged 1 commit into from
Jul 19, 2017

Conversation

pedrottimark
Copy link
Contributor

Summary

Benefits of distinguishing config properties which don’t change (depend only on options and defaults) from arguments which do change during recursive descent through data structures:

  • concise clear signature for core functions
  • correct complete consistent API for plugins (but one step at a time :)

Converting from options to config in an even more functional lazy evaluation pattern solves a recent accidental performance regression (always iterating theme colors, even for top-level basic values).

Removed a readbump in printList, printMap, printObject, printSet: logic for final comma had been combined with edge spacing and indent instead of in parallel with logic for non-final comma.

Make yourself a cup of coffee or tea before you review this diff.

Test plan

Jest, if the Continuous Irritation system allows

@cpojer
Copy link
Member

cpojer commented Jul 19, 2017

Woah, that is significantly simpler! Nice work. Do you have any perf numbers?

@codecov-io
Copy link

Codecov Report

Merging #4076 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4076      +/-   ##
==========================================
+ Coverage   60.42%   60.45%   +0.02%     
==========================================
  Files         196      196              
  Lines        6769     6769              
  Branches        6        6              
==========================================
+ Hits         4090     4092       +2     
+ Misses       2676     2674       -2     
  Partials        3        3
Impacted Files Coverage Δ
packages/pretty-format/src/index.js 98.03% <100%> (+0.04%) ⬆️
packages/jest-config/src/normalize.js 80.97% <0%> (-0.11%) ⬇️
packages/jest-config/src/index.js 0% <0%> (ø) ⬆️
packages/jest-cli/src/get_changed_files_promise.js 100% <0%> (ø) ⬆️
packages/jest-haste-map/src/index.js 92.96% <0%> (+0.33%) ⬆️
packages/jest-changed-files/src/git.js 92.59% <0%> (+2.93%) ⬆️

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 45ceff8...632afb7. Read the comment docs.

@pedrottimark
Copy link
Contributor Author

Thank you for reminding. Measurements of an experimental version in which proposed with this change is compared to baseline with all other upcoming proposed changes except this change.

  • The first row was accidental discovery that there had been a recent performance regression.
  • Other than that, I breathed a sigh of relief that accessing object properties doesn’t hurt.
scenario plugins proposed baseline ratio
basic 0 5863 22992 0.255
basic 9 23869 65253 0.366
complex 0 192674 237179 0.812
complex 9 584595 624248 0.936
geo 0 27962412 27013068 1.035
geo 9 60198020 60391597 0.997
react-element 9 772529 822932 0.939
react-object 9 658507 724195 0.909

Measuring performance is still spooky stuff to me. Even taking geometric mean of 3 runs from restart without wifi occasionally has surprising variation.

Because pretty-format has always seemed very careful not to allocate unnecessary memory, I looked at heapUsed for this batch but sometimes it varied wildly when the times didn’t.

@cpojer cpojer merged commit 2b2b9ba into jestjs:master Jul 19, 2017
@cpojer
Copy link
Member

cpojer commented Jul 19, 2017

Yap, it's close enough.

@pedrottimark pedrottimark deleted the pretty-format-config branch July 19, 2017 15:37
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants