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 crlf (windows) end of lines forcing to lf (unix) #1711

Merged
merged 1 commit into from Oct 20, 2015

Conversation

Projects
None yet
3 participants
@deformhead
Contributor

deformhead commented Oct 15, 2015

No description provided.

@patrickkettner

This comment has been minimized.

Show comment
Hide comment
@patrickkettner

patrickkettner Oct 15, 2015

Member

@deformhead thanks for the PR!
is this an issue somewhere, or a preventative measure

Member

patrickkettner commented Oct 15, 2015

@deformhead thanks for the PR!
is this an issue somewhere, or a preventative measure

@deformhead

This comment has been minimized.

Show comment
Hide comment
@deformhead

deformhead Oct 15, 2015

Contributor

Both. We use it for a project at work, and we need to build a vendors file with all libraries installed with npm by a Jenkins (unix) server. Since some javascript files are in CRLF, the build fails.

So if someone works on this kine of environment, it may be a problem.

Contributor

deformhead commented Oct 15, 2015

Both. We use it for a project at work, and we need to build a vendors file with all libraries installed with npm by a Jenkins (unix) server. Since some javascript files are in CRLF, the build fails.

So if someone works on this kine of environment, it may be a problem.

@patrickkettner

This comment has been minimized.

Show comment
Hide comment
@patrickkettner

patrickkettner Oct 15, 2015

Member

Sure, what I am trying to say is that this will only prevent it from happening in the future. If you see this issue on a file(s) in the repo currently - what are they, and can you update them within this PR?

Member

patrickkettner commented Oct 15, 2015

Sure, what I am trying to say is that this will only prevent it from happening in the future. If you see this issue on a file(s) in the repo currently - what are they, and can you update them within this PR?

@deformhead

This comment has been minimized.

Show comment
Hide comment
@deformhead

deformhead Oct 16, 2015

Contributor

Ok, my mistake : non-LF files was just media files ( .ai, .eps, .pdf and .png ), so no worries.

Besides, I updated my PR to prevent LF files to be changed ( preventative measure, so ).

If you need me to update this PR ( add or remove specific file patterns... ), let me know.

Contributor

deformhead commented Oct 16, 2015

Ok, my mistake : non-LF files was just media files ( .ai, .eps, .pdf and .png ), so no worries.

Besides, I updated my PR to prevent LF files to be changed ( preventative measure, so ).

If you need me to update this PR ( add or remove specific file patterns... ), let me know.

@ryanseddon

This comment has been minimized.

Show comment
Hide comment
@ryanseddon

ryanseddon Oct 17, 2015

Member

Could we just force all to LF rather than target certain file extensions?

* text eol=lf
Member

ryanseddon commented Oct 17, 2015

Could we just force all to LF rather than target certain file extensions?

* text eol=lf
@deformhead

This comment has been minimized.

Show comment
Hide comment
@deformhead

deformhead Oct 17, 2015

Contributor

That was my first call, but media files was seen as text ( converting media files to LF brake them ).

So, two options :

I target files to be converted like I have done, but making a list is ugly, I agree.

Otherwise, I keep my first idea to keep LF on all text files, but I need to be certain which files do you want to "escape" from being a text ( which need a listing too, but smaller ). Only media files ? Most important, we have to keep in mind that we need to update this .gitattributes file if another media type come to avoid it to be convert and to be broken.

So, option one is ugly but safier and option two is better but tricky.

Contributor

deformhead commented Oct 17, 2015

That was my first call, but media files was seen as text ( converting media files to LF brake them ).

So, two options :

I target files to be converted like I have done, but making a list is ugly, I agree.

Otherwise, I keep my first idea to keep LF on all text files, but I need to be certain which files do you want to "escape" from being a text ( which need a listing too, but smaller ). Only media files ? Most important, we have to keep in mind that we need to update this .gitattributes file if another media type come to avoid it to be convert and to be broken.

So, option one is ugly but safier and option two is better but tricky.

@patrickkettner

This comment has been minimized.

Show comment
Hide comment
@patrickkettner

patrickkettner Oct 17, 2015

Member

How are you getting the files on your build server

On Oct 17, 2015, at 12:17 AM, deformhead notifications@github.com wrote:

That was my first call, but media files was seen as text ( converting media files to LF brake them ).

So, two options :

I target files to be converted like I have done, but making a list is ugly, I agree.

Otherwise, I keep my first idea to keep LF on all text files, but I need to be certain which files do you want to "escape" from being a text ( which need a listing too, but smaller ). Only media files ? Most important, we have to keep in mind that we need to update this .gitattributes file if another media type come to avoid it to be convert and to be broken.

So, option one is ugly but safier and option two is better but tricky.


Reply to this email directly or view it on GitHub.

Member

patrickkettner commented Oct 17, 2015

How are you getting the files on your build server

On Oct 17, 2015, at 12:17 AM, deformhead notifications@github.com wrote:

That was my first call, but media files was seen as text ( converting media files to LF brake them ).

So, two options :

I target files to be converted like I have done, but making a list is ugly, I agree.

Otherwise, I keep my first idea to keep LF on all text files, but I need to be certain which files do you want to "escape" from being a text ( which need a listing too, but smaller ). Only media files ? Most important, we have to keep in mind that we need to update this .gitattributes file if another media type come to avoid it to be convert and to be broken.

So, option one is ugly but safier and option two is better but tricky.


Reply to this email directly or view it on GitHub.

@deformhead

This comment has been minimized.

Show comment
Hide comment
@deformhead

deformhead Oct 17, 2015

Contributor

What do you mean ?

The fail on our build is not from a Modernizr mistake, at the end, but from another library, sorry for my first comment.

Now, I'm only dealing with forcing text files ( and text only ) to be LF, in this PR.

Contributor

deformhead commented Oct 17, 2015

What do you mean ?

The fail on our build is not from a Modernizr mistake, at the end, but from another library, sorry for my first comment.

Now, I'm only dealing with forcing text files ( and text only ) to be LF, in this PR.

@patrickkettner

This comment has been minimized.

Show comment
Hide comment
@patrickkettner

patrickkettner Oct 18, 2015

Member

we should be able to use a .gitattributes file directly in the media/ folder. it will only apply to those files. then keep your original change in the main .gitattributes file

Member

patrickkettner commented Oct 18, 2015

we should be able to use a .gitattributes file directly in the media/ folder. it will only apply to those files. then keep your original change in the main .gitattributes file

@deformhead

This comment has been minimized.

Show comment
Hide comment
@deformhead

deformhead Oct 19, 2015

Contributor

@patrickkettner, I updated my PR in this way.

Contributor

deformhead commented Oct 19, 2015

@patrickkettner, I updated my PR in this way.

ryanseddon added a commit that referenced this pull request Oct 20, 2015

Merge pull request #1711 from deformhead/master
fix crlf (windows) end of lines forcing to lf (unix)

@ryanseddon ryanseddon merged commit 99ff51a into Modernizr:master Oct 20, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ryanseddon

This comment has been minimized.

Show comment
Hide comment
Member

ryanseddon commented Oct 20, 2015

Thanks @deformhead

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