Add end of file new line check fix #496 #5583
Conversation
jenkins/check_file_eol.sh
Outdated
| fi | ||
|
|
||
| # Check that each file ends in a newline character | ||
| tail -c1 $1 | od -x | grep '000a' |
There was a problem hiding this comment.
The previous step redirects output from grep to $ERRFILETEMP. Do we want to do that here too?
boingoing
left a comment
There was a problem hiding this comment.
Seems fine to me. This is only going to catch standard newline characters, do we ever want to support Unicode newlines like U+2029 and U+2028, 0x85, etc? I'd guess no but just want to leave a note here.
|
This will probably cause a bunch of files to start failing CI -- should we include those updates in this patch?
Note: historic mistakes will be caught the next time someone edits that file, and it will be at least a minor annoyance to have to fix something that they didn't cause. (This might be fine, it's just a matter of distributing versus front-loading pain.) |
805b504 to
a02b54b
Compare
|
@dilijev I ran a script to correct all the mistakes. One question off of the back of this - should .wast files be excluded? |
|
@Cellule or @MikeHolman would know about wast files. I actually don't think there's any reason to block landing this on the number of files that currently violate the check without being flagged yet. I think we should just merge the check as-is and either follow up with a "fix all the things" commit, or just let everyone gradually fix files where the check complains. Edit: I see you already fixed all the files in addition to the wast files -- so let's wait to hear back and then decide. I'm surprised there weren't more issues actually. :) |
|
@dilijev The second commit here fixed this for all files that would have violated it at the time I added it. My question was more that as I followed the exclusions included in check_eol.sh file .wasm files are excluded from the check but .wast files are not and I was wondering if that's a mistake. |
|
It’s fine to include .wast files, but .wasm files are binary so they should be excluded from the check. |
|
@dilijev I'm assuming this is good to merge then per comments above and won't update unless you ask me to. |
|
The .wast files in test/WasmSpec* should be excluded, those files are not maintained by us, we just take them from the wasm spec repo. I don't want to have to edit the files every time I make an update. |
|
Updated to exclude .wast. Note I haven't re-run the script to fix existing exceptions for any introduced since I did it before (as I'd accidentally deleted that script and didn't have time to re-write it at the moment). Hopefully there aren't any or at least aren't many. |
|
LGTM |
Merge pull request #5583 from rhuanjl:endOfFile Per issue #496 augment the existing line ending check to also check to ensure that each file ends in a newline. Note, per logic in check_eol.sh this test is run only on: 1. Files edited in the current PR 2. Files that are not JS test files OR any cmd, baseline, wasm, vcxproj or sln files Also note: 1. As this runs only on files edited din current PR it won't catch any historic mistakes 2. It doesn't check JS test files - this seemed odd to me but chose to leave it CC @dilijev
Per issue #496 augment the existing line ending check to also check to ensure that each file ends in a newline.
Note, per logic in check_eol.sh this test is run only on:
Also note:
CC @dilijev