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

Use read-pkg-up over pkg-up. #680

Merged
merged 5 commits into from
Dec 15, 2016
Merged

Use read-pkg-up over pkg-up. #680

merged 5 commits into from
Dec 15, 2016

Conversation

wtgtybhertgeghgtwtg
Copy link
Contributor

Closes #675.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

I can't seem to find why this is failing. Does anyone with more experience with rule-tester have any insight?

Copy link
Collaborator

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @wtgtybhertgeghgtwtg! Thanks for the PR and sorry for the delay.

I made comments that should help you fix the tests :)

@@ -1,18 +1,14 @@
import fs from 'fs'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this import, now that it is not used?

try {
const packageContent = JSON.parse(fs.readFileSync(filepath, 'utf8'))
const pkg = readPkgUp.sync({cwd: context.getFilename(), normalize: false})
const packageContent = JSON.parse(pkg.pkg)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem that makes the build fail is that getDependencies is now returning something else. pkg.pkg is now an object, and not a string that remains to be parsed. By attempting to parse it you are causing an error, which will be cached and make the function return null.

This works better:

    const pkg = readPkgUp.sync({cwd: context.getFilename(), normalize: false})
    if (!pkg || !pkg.pkg) {
      return null
    }

    const packageContent = pkg.pkg

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I don't know how I missed that.

@coveralls
Copy link

coveralls commented Dec 13, 2016

Coverage Status

Coverage remained the same at 94.86% when pulling dfc381c on wtgtybhertgeghgtwtg:master into bfdc2bb on benmosher:master.

@jfmengels
Copy link
Collaborator

jfmengels commented Dec 14, 2016

Thanks for the changes :)

LGTM, but do you know why the whole file is considered a having been changed in that last commit, instead of only the modified lines? That's pretty odd 🤔
If possible, I would love to have only the lines that have really changed be marked as changed, for debugging purposes later on. If you feel like looking at that, that'd be great. Otherwise, it's not that big of a deal, but surprising still.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

wtgtybhertgeghgtwtg commented Dec 14, 2016

I accidently saved it as CRLF instead of LF. That should be fixed now.

Copy link
Collaborator

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, the diff is much easier to read now 😅

LGTM, thanks for your work on this :)

I'll give some time to @benmosher and @ljharb to review, and I'll merge this in a few days if they dontt have the time to look at it.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

Thank you for helping me with this Pull Request.

@coveralls
Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage remained the same at 94.86% when pulling 1388a8d on wtgtybhertgeghgtwtg:master into bfdc2bb on benmosher:master.

@benmosher benmosher merged commit 8df3c9a into import-js:master Dec 15, 2016
@benmosher
Copy link
Member

thanks @wtgtybhertgeghgtwtg!

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

Thank you for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants