mootools / mootools-core

MooTools Core Repository
https://mootools.net
2.65k stars 511 forks source link

removing linebreak rule from ESLint for OS compatibility #2770

Closed esr360 closed 8 years ago

esr360 commented 8 years ago

Solves the following issue: https://github.com/mootools/mootools-core/issues/2769

SergioCrisostomo commented 8 years ago

I'm ok with this ESLint change.

timwienk commented 8 years ago

I'm not. The ESLint settings are for our source files, and we require all our source files to only have linefeeds as newline characters.

What Grunt task are you getting errors from about CRLF newline characters? I don't remember the built files being ESLinted at all (and I don't really see why we would, if we do).

What we could do, however, is set grunt.util.linefeed = '\n'; in the Gruntfile.js. That should make Grunt use only linefeeds when building.

esr360 commented 8 years ago

I am using the nocompat task. I require an unminified copy of the compiled file ideally for my project, otherwise you are correct I could use the regular compat task which may not use the eslint task.

Currently I have these tasks to handle this issue for me:

            mooTools_windows: {
                src: 'assets/vendor/MooTools-Core/.eslintrc',
                overwrite: true, 
                replacements: [{
                    from: 'linebreak-style: [2, unix]',
                    to:   'linebreak-style: [2, windows]'
                }]
            },
            mooTools_reset: {
                src: 'assets/vendor/MooTools-Core/.eslintrc',
                overwrite: true, 
                replacements: [{
                    from: 'linebreak-style: [2, windows]',
                    to:   'linebreak-style: [2, unix]'
                }]
            }

Then further down I have something like:

        if (process.platform === 'win32') {
             assetTasks.push('replace:mooTools_windows');
        };

Which seems crazy but in order to fit within my workflow it is a neccessery step currently if I wish to use MooTools Core on Windows.

timwienk commented 8 years ago

The steps you mentioned should indeed not be necessary, but I think I understand the issue now, so thanks for explaining.

I assumed that you got the errors because the built file was ESLinted, which seemed awkward to me (since, like I mentioned, the built file should not be ESLinted at all). However, you just confirmed that the errors occur during the built itself.

I'm fairly certain the problem is not with Grunt or ESLint, but with the way Git handles text files by default. Git's default setting of core.eol = native makes it so upon checkout text files are converted to use CRLF line endings in windows working directories, even if they use LF line endings in the repository. I'm sure this avoids a lot of problems when editing files, but it creates problems like these when enforcing consistency within files.

So, instead of changing the ESLint settings, we should change the way Git handles these files. There is a local way of doing this, using your local config, but instead I think we should add .gitattributes file to make sure it's done correctly everywhere.

Additionally, I think we should still add the grunt.util.linefeed = '\n', so the built file won't have a mix of LF and CRLF line endings afte this change.

I've just pushed https://github.com/timwienk/mootools-core/commit/cba3825 https://github.com/timwienk/mootools-core/commit/70afaa8 to my own repository. Is it possible for you to test whether that fixes your issues?

If you're pulling the change to an existing clone of the repository, you will have to re-checkout the files after pulling the commit so the line ending change takes effect. From inside the repository, this should work:

$ rm .git/index
$ git reset --hard

Or, in a normal Windows prompt:

> del .git\index
> git reset --hard
DimitarChristoff commented 8 years ago

that's a bit moot, though. when you install msysgit it asks you specifically about line endings and picking "leave as is" seems like the only solution that works cross platform as most Windows devs normally have a Linux based CI

On Tuesday, 16 August 2016, Tim Wienk notifications@github.com wrote:

The steps you mentioned should indeed not be necessary, but I think I understand the issue now, so thanks for explaining.

I assumed that you got the errors because the built file was ESLinted, which seemed awkward to me (since, like I mentioned, the built file should not be ESLinted at all). However, you just confirmed that the errors occur during the built itself.

I'm fairly certain the problem is not with Grunt or ESLint, but with the way Git handles text files by default. Git's default setting of core.eol = native makes it so upon checkout text files are converted to use CRLF line endings in windows working directories, even if they use LF line endings in the repository. I'm sure this avoids a lot of problems when editing files, but it creates problems like these when enforcing consistency within files.

So, instead of changing the ESLint settings, we should change the way Git handles these files. There is a local way of doing this, using your local config, but instead I think we should add .gitattributes file to make sure it's done correctly everywhere.

Additionally, I think we should still add the grunt.util.linefeed = '\n', so the built file won't have a mix of LF and CRLF line endings afte this change.

I've just pushed timwienk@cba3825 https://github.com/timwienk/mootools-core/commit/cba3825 to my own repository. Is it possible for you to test whether that fixes your issues?

If you're pulling the change to an existing clone of the repository, you will have to re-checkout the files after pulling the commit so the line ending change takes effect. From inside the repository, this should work:

git rm --cached -r *.js Grunt Source Specs git reset --hard

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mootools/mootools-core/pull/2770#issuecomment-240077750, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHSzBAtWJ89V8xNoyaBubPKJeyWhyVeks5qgaF_gaJpZM4JjvL_ .

Dimitar Christoff

"JavaScript is to JAVA what hamster is to ham" @D_mitar - https://github.com/DimitarChristoff

timwienk commented 8 years ago

when you install msysgit it asks you specifically about line endings

I see. I guess that question will set core.autocrlf accordingly.

In my commit I also set * text=auto in the .gitattributes file. That will make it so that (independent of the core.aotocrlf value) all files are checked out with CRLF line endings when core.crlf = crlf (or core.crlf = native on Windows). From your comment, Dimitar, that seems to not be a good idea.

I've replaced my commit with https://github.com/timwienk/mootools-core/commit/70afaa8. That way it only changes behaviour for JavaScript files: they will always be checked out with LF line endings.

DimitarChristoff commented 8 years ago

:+1:

On Tuesday, 16 August 2016, Tim Wienk notifications@github.com wrote:

when you install msysgit it asks you specifically about line endings

I see. I guess that question will set core.autocrlf accordingly.

In my commit I also set * text=auto in the .gitattributes file. That will make it so that (independent of the core.aotocrlf value) all files are checked out with CRLF line endings when core.crlf = crlf (or core.crlf = native on Windows). From your comment, Dimitar, that seems to not be a good idea.

I've replaced my commit with timwienk@70afaa8 https://github.com/timwienk/mootools-core/commit/70afaa8. That way it only changes behaviour for JavaScript files: they will always be checked out with LF line endings.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mootools/mootools-core/pull/2770#issuecomment-240235748, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHSzCcnYZd1kel9TiThx7551cmFrTceks5qgiPKgaJpZM4JjvL_ .

Dimitar Christoff

"JavaScript is to JAVA what hamster is to ham" @D_mitar - https://github.com/DimitarChristoff

SergioCrisostomo commented 8 years ago

Nice fix and input here @timwienk. 👍

timwienk commented 8 years ago

Since the changes I made aren't in actual functional code, I'm going to go ahead and push them to the master branch.

@esr360, if you have time, it'd be great if you can let us know whether this solved your problems. If not, we can try and look further.

Do remember that, when pulling to an existing clone of the repository in an environment that converts to CRLF, you will have to re-checkout the files after pulling the commit so the line ending change takes effect. From inside the repository, this should work:

$ rm .git/index
$ git reset --hard

Or, in a normal Windows prompt:

> del .git\index
> git reset --hard
esr360 commented 8 years ago

@timwienk Thank you very much, your change seems to have solved the issue for me :)

timwienk commented 8 years ago

Great, thanks for letting us know!