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

Add diff-sequences package #5407

Merged
merged 11 commits into from
Feb 7, 2018
Merged

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Jan 27, 2018

This generic package has specific goals for jest-diff in a future pull request:

  • Minimize time and space when expected and received values have many differences.
  • Minimize code differences between --expand and --no-expand options.

Here are recommended steps to review this pull request:

  1. Read README.md especially the examples of callback functions

  2. Refer to An O(ND) Difference Algorithm and Its Variations by Eugene W. Myers

    • Read up through Figure 1 on page 3 and make sure to understand the “edit graph” analogy
    • Read pseudo code in Figure 2 on page 6
    • Look at Figure 3 Furthest reaching paths on page 7
    • Read pseudo code on pages 11 and 12
  3. Read tests in index.test.js

  4. The first time you read code in index.js skip the two Overlappable functions

Overview of cost:

  • time and space optimization: skip diagonals in which paths cannot ever overlap

  • heap used is two arrays of small integers: each has max length of one more than length of shorter sequence

  • stack depth is at most logarithm of number of differences [EDIT optimization: does not recurse whenever number of changes is equal to number of items in either half of divided interval, therefore cannot contain any common items]

Test plan

44 tests [EDIT: plus 4 and forgot to say 100% coverage]

@codecov-io
Copy link

codecov-io commented Jan 27, 2018

Codecov Report

Merging #5407 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5407   +/-   ##
=======================================
  Coverage   61.35%   61.35%           
=======================================
  Files         205      205           
  Lines        6925     6925           
  Branches        4        3    -1     
=======================================
  Hits         4249     4249           
  Misses       2675     2675           
  Partials        1        1

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 0110101...645994d. Read the comment docs.

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jan 27, 2018

Residue:

  • [EDIT: Caleb gave me the correct solution to type cast intention invalid arg as any] Flow reports This type is incompatible with the expected param type of for 3 of the invalid arg tests. I used // $FlowFixMe comment preceding the call, but would like to replace it with more specific // flowlint-line TODO:off if I can find what is the name of that error.

  • When I tried module.exports = diffSequences; following the example of 3 packages which have browser builds, ESLint reported No default export found maybe because index.js does not import anything?

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jan 27, 2018

[EDIT: No need to export types of input and output callback functions, because type of default export includes its args, true?] Thinking ahead to when jest-diff replaces its diff dependency with this package, should I move the exported types IsCommon and FoundSubsequence for callback functions to a file in the types folder? And define a type like FindSubsequences for the default export?

For other projects to type check this package as a dependency means that it will need definitions in flow-typed and definitely-typed?

@cpojer cpojer merged commit ad91d0a into jestjs:master Feb 7, 2018
@cpojer
Copy link
Member

cpojer commented Feb 7, 2018

This is a huge diff but it looks really really good.

  • Can you use ES exports instead?
  • Flow-typed makes sense if you wanna use it outside of this repo.

@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 12, 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