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

Minor depth calculation fixes #3

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

stephank
Copy link

@stephank stephank commented Jan 21, 2019

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
Copy link

codecov-io commented Jan 21, 2019

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) ⬇️
src/YarnLock.php 99% <100%> (+0.01%) 45 <0> (ø) ⬇️

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
Copy link

Pull Request Test Coverage Report for Build 10

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 99.597%

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

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 10

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 99.597%

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

💛 - Coveralls

@PRGfx
Copy link
Member

PRGfx commented Jan 21, 2019

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'

Sign up for free to 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.

4 participants