mindscreen / yarnlock

A php-package for parsing and evaluating the yarn.lock format
5 stars 5 forks source link

Minor depth calculation fixes #3

Open stephank opened 5 years ago

stephank commented 5 years ago

Hi! Thanks for creating this package, it's really useful!

bbe2e0e fixes an issue where calculateDepth() (with no roots) fails because resolves = null. The check for count($p->getResolves()) always fails, because resolves couldn't ever be an empty array (it's either null or non-empty).

aa463f0 just ensures calculateDepth() has run when you use getPackagesByDepth(). Otherwise, the latter always returns an empty array. (getDepth() already does the same.)

5558b05 and 3c77397 are readme fixes. It seems like the readme was outdated, maybe? There's no need to parse package.json, and just doing getPackagesByDepth(0) works fine for me.

codecov-io commented 5 years ago

Codecov Report

Merging #3 into master will decrease coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master       #3      +/-   ##
============================================
- Coverage     99.65%   99.65%   -0.01%     
+ Complexity      121      120       -1     
============================================
  Files             3        3              
  Lines           290      289       -1     
============================================
- Hits            289      288       -1     
  Misses            1        1
Impacted Files Coverage Δ Complexity Δ
src/Package.php 100% <ø> (ø) 25 <0> (-1) :arrow_down:
src/YarnLock.php 99% <100%> (+0.01%) 45 <0> (ø) :arrow_down:

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 e4308b9...3c77397. Read the comment docs.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 10


Totals Coverage Status
Change from base Build 9: -0.05%
Covered Lines: 247
Relevant Lines: 248

💛 - Coveralls
coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 10


Totals Coverage Status
Change from base Build 9: -0.05%
Covered Lines: 247
Relevant Lines: 248

💛 - Coveralls
PRGfx commented 5 years ago

Thank you for your contribution.

Calculating depths without a set of root dependencies generally will return wrong results. Consider the following example: minimal package.json

{
  "dependencies": {
    "prop-types": "^15.6.2",
    "react": "^16.7.0"
  }
}

Usually we will assume all packages that are not required by other packages in the yarn.lock to be root dependencies. As prop-types is required by react, it will calculate it's depth by the "most shallow" parent dependency. In this case getPackagesByDepth(0) will only return the react package, while one would expect prop-types to be returned as well.

Admittedly getDepth() ignores this problem as well - as you mentioned - and would yield (potentially) wrong results, if used before manually calculating dependencies from root-packages.

I accept the other changes you proposed, but I insist on having any kind of explanation in the readme regarding the "need" for calculateDepth() in certain cases. Ideally I could imagine something like resetDepth() resetting the depthCalculated property and a test showing something like

$yarnLock->getPackagesByDepth(0); // 'react'
$yarnLock->resetDepth();
$yarnLock->calculateDepth(
  array_map(
    function($name) use ($yarnLock) {
      return $yarnLock->getPackagesByName($name)[0];
    },
    ['react', 'prop-types']
  )
);
$yarnLock->getPackagesByDepth(0); // 'react', 'prop-types'