octobercms / october

Self-hosted CMS platform based on the Laravel PHP Framework.
https://octobercms.com/
Other
11.03k stars 2.21k forks source link

Wrong line endings (CRLF) in some files #725

Closed galaxy4public closed 9 years ago

galaxy4public commented 10 years ago

Some files in the october/modules/backend/formwidgets/codeeditor/assets/vendor/ace/ directory have weird line-endings. For example, the mode-diff.js file there has its header with Unix line endings (LF), the middle of the file with Windows line endings (CRLF), and the last section is again using Unix line endings (LF).

It would be nice to have consistency in the line endings across the whole repository. If you could run something like that on the ace directory and then commit the updated files it would be much appreciated:

find ./october/modules/backend/formwidgets/codeeditor/assets/vendor/ace -type f -name '*.js' -exec perl -pi -e 's,\r+$,,g' '{}' \;

Updated Oct 29: made the one-liner a bit safer to fix the line endings only (previously it was removing the CR chars even in the middle of the string, which should not happen but still is a bug).

Thanks!

galaxy4public commented 10 years ago

Actually, there are just 4 such weird files in the whole repository:

october/modules/backend/formwidgets/codeeditor/assets/vendor/ace/mode-diff.js
october/modules/backend/formwidgets/codeeditor/assets/vendor/ace/mode-jsoniq.js
october/modules/backend/formwidgets/codeeditor/assets/vendor/ace/mode-xquery.js
october/modules/backend/formwidgets/codeeditor/assets/vendor/ace/worker-html.js

I suspect that they were introduced to repository before the "* text=auto" got into .gitattributes (otherwise these files would have been fixed on commit).

galaxy4public commented 10 years ago

OK, I went to ajaxorg/ace-build and looked inside the src directory (it looks like the original directory that was copied over to october/modules/backend/formwidgets/codeeditor/assets/vendor/ace). The line endings there are all mixed up. I guess this is due to the way they "build" their package: there are multiple JS files compiled into a smaller set of files, hence if one file had Unix line endings and another file has DOS/Windows line endings once they are merged together we are getting that inconsistency.

Since OctoberCMS doesn't track the original ace-build repo at the moment I still think that fixing the line endings with the provided one liner is the best approach. We may also want to sync the whole directory from the ace-build at the same time - there were a lot of updates since the previous sync.

Also it is worth to mention that currently OctoberCMS includes lots of modes and themes for ACE, but there is no way to tweak that in the current infrastructure. Maybe we just need to keep the modes, themes, and workers we are interested in: e.g. mode-{css,diff,html,javascript,php,plain_text}.js, worker-{css,html,javascript,php}.js, and theme-twilight.js? Anyway, this part is really optional since people who are concerned re: this will find a way to make it work :)

daftspunk commented 10 years ago

Theme can be tweaked via the Backend User Preferences, so we'll keep the themes, but I agree. Will update as part of #680

galaxy4public commented 10 years ago

@daftspunk, thank you for updating and cleaning it up. However, I strongly disagree with the minification of JavaScript in a repository: the reason is simple - we take something that is manageable and can be worked on and turn it into unmanageable pile of something unpleasant. Further, I think that any minification and post-processing is a task for the deployment process, not the development. It would be great if we could revert back the minification please.

daftspunk commented 10 years ago

Yes I think so too, I will put the non minified version in there. I was so impressed with the reduced disk size of removing the other modes I think I may have gone too far ;-)

galaxy4public commented 10 years ago

@daftspunk, please remember that every time you import something from a 3rd party source to keep your repository clean and sane you need to filter files through something like:

perl -pi -e 's,\r+$,,g' file.ext

The above is the safe command replacing the CRLF line-endings with LF. However, I think it's safe to assume that we should not expect \r anywhere in OctoberCMS and ACE editor is known for having weird chunks of \r incorporated mid-line, therefore for the ace directory I'd suggest to run:

find october/modules/backend/formwidgets/codeeditor/assets/vendor/ace -type f -name '*.js' -exedir perl -pi -e 's,\r,,g' '{}' \;

The reason for this comment is that I see that normal versions of ACE js files re-appeared on the develop branch, yet, we have \r in some of them (e.g. mode-diff.js)