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

Fix error when an absolute path is given #80

Merged
merged 1 commit into from
Mar 12, 2020

Conversation

nweldev
Copy link
Contributor

@nweldev nweldev commented Mar 5, 2020

When an absolute path is given (e.g. via lint-staged), node-ignore was throwing a RangeError path should be a path.relative()'d string.
see https://github.com/kaelzhang/node-ignore#1-pathname-should-be-a-pathrelatived-pathname

This is only a "quick win" to fix this issue.
As prepareFileList was already returning an absolute property based on the cwd, this commit only introduce a new relative property, which is also based on the cwd.
No new specific test was written for this use case.

Close #79

When an absolute path is given (e.g. via `lint-staged`), `node-ignore`
was throwing a RangeError `path should be a path.relative()'d string`.
see https://github.com/kaelzhang/node-ignore#1-pathname-should-be-a-pathrelatived-pathname

This is only a "quick win" to fix this issue.
As `prepareFileList` was already returning an `absolute` property based
on the cwd, this commit only introduce a new `relative` property, which
is also based on the cwd.
No new specific test was written for this use case.
Copy link
Collaborator

@DavidAnson DavidAnson left a comment

Choose a reason for hiding this comment

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

This makes sense, thanks!

You say it’s a quick fix, but it looks pretty reasonable to me. Did you have any concerns about this?

@@ -110,6 +110,7 @@ function prepareFileList(files, fileExtensions, previousResults) {
return flatten(files).map(function (file) {
return {
original: file,
relative: path.relative(process.cwd(), file),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you capture the call to cwd in a const outside the map, please?

@DavidAnson
Copy link
Collaborator

I would love to add tests here, but I think you didn’t because it’s not clear how to do so in a platform-independent manner, right?

@DavidAnson DavidAnson merged commit 6e9849c into igorshubovych:master Mar 12, 2020
@DavidAnson
Copy link
Collaborator

I found a way to add tests that should be platform-independent and merged this PR.

@nweldev
Copy link
Contributor Author

nweldev commented Mar 17, 2020

Hi @DavidAnson. Sorry I forgot about this PR. Thansk for the merge.
To answer your question, I confirm that I didn't add tests because it wasn't clear enough how to write one, but also because I didn't have a lot of time for this issue. My only concern was my lack of time and tests 😅

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.

Bug: Can't handle absolute paths
3 participants