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

Checking for paths.length after filtering #43

Closed
wants to merge 8 commits into from

Conversation

mickvangelderen
Copy link
Contributor

Hey @sindresorhus,

I'm not sure if the paths.length === 0 check is before the filter(fileExist) check on purpose but to me it makes sense to put it after the filter.

If I want a file to be removed but it doesn't exist in the first place then that is perfectly fine with me. There shouldn't be an error (which the windows binary yields).

Also I set the line endings to LF in the .gitattributes file and made some code simplifications (at least I think so).

Let me know what you think :)

@@ -1,3 +1,3 @@
* text=auto
* text eol=lf
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't work like you think. It's super flawed and really means "convert everything to text and with lf newlines" instead of meaning "convert text to use lf newlines". So it will ruin anything binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about changing it to

*.js test eol=lf

Copy link
Owner

Choose a reason for hiding this comment

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

Why do you need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partly because of the .editorconfig and mostly because when I cloned and ran the test, I got an error for every line telling me it encountered a CRLF but expected a LF. I did not have an editor config plugin installed at first.

@sindresorhus
Copy link
Owner

I'm not sure if the paths.length === 0 check is before the filter(fileExist) check on purpose but to me it makes sense to put it after the filter.

Probably an oversight when globbing was added. It should definitely check the final paths array.

@@ -1,3 +1,3 @@
* text=auto
Copy link
Owner

Choose a reason for hiding this comment

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

This should still be preserved.

@sindresorhus
Copy link
Owner

Thank you! Merry xmas 🎄

@mickvangelderen
Copy link
Contributor Author

Haha thanks you! Happy hollidays!

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.

None yet

2 participants