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

Refactor names and delimiters of complex values in pretty-format #3986

Merged
merged 2 commits into from
Jul 8, 2017

Conversation

pedrottimark
Copy link
Contributor

Summary

There are some inconsistencies in names of complex types for various options:

  • specific case for empty list is Arguments [] but general case with min: true is []
  • specific case for empty array is Array [] and so on, but general case with min: true is []
  • when hitMaxDepth for typed array is [Array] instead of [Uint32Array] and so on
  • when hitMaxDepth for constructed object is [Object] instead of '[' + (val.constructor ? val.constructor.name : 'Object') + ']' and I’m not sure tests cover this case

If y’all are willing to approve breaking changes to edge cases, I can submit an additional PR.

Whether or not, this PR applies advice from Kent Beck: move similar elements closer together.

  • Move empty arguments and array from printBasicValue to printComplexValue
  • Move complex value name and delimiters from helpers to printComplexValue
  • Therefore delete redundant helpers printArguments and printArray

Effect of refactoring on performance:

  • Time to print empty arguments or array increases similar to empty object. I think the reason for the original logic is to avoid computing indent when it isn’t used for those values. It has been confusing to me that the special cases in printBasicValue (where I don’t expect to find arguments or arrays) are inconsistent with the general cases in printCompexValue :(
  • Time saving from evaluating isToStringedArrayType only 1 time instead of 2 and calling printList directly for arguments and array is subtle, within the precision of measurement.

Test plan

Jest

@cpojer
Copy link
Member

cpojer commented Jul 7, 2017

Seems like circle is unhappy.

@pedrottimark
Copy link
Contributor Author

In case it helps, I will push another commit to fix this unrelated problem:

$ eslint . --cache --ext js,md

/Users/mark/gitforks/jest/packages/jest-haste-map/src/index.js
  831:1  error  Delete `······`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

error Command failed with exit code 1.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@e6704bf). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3986   +/-   ##
=========================================
  Coverage          ?   59.97%           
=========================================
  Files             ?      196           
  Lines             ?     6814           
  Branches          ?        6           
=========================================
  Hits              ?     4087           
  Misses            ?     2724           
  Partials          ?        3
Impacted Files Coverage Δ
packages/jest-haste-map/src/index.js 92.96% <ø> (ø)
packages/pretty-format/src/index.js 97.98% <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 e6704bf...43a4664. Read the comment docs.

@cpojer cpojer merged commit a0def46 into jestjs:master Jul 8, 2017
@cpojer
Copy link
Member

cpojer commented Jul 8, 2017

Happy to accept a PR for breaking changes when you are making edge cases more consistent.

@pedrottimark pedrottimark deleted the refactor-complex-value branch July 11, 2017 19:02
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
…tjs#3986)

* Refactor names and delimiters of complex values in pretty-format

* Delete extra spaces
@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