-
Notifications
You must be signed in to change notification settings - Fork 139
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
Stop changing 'encoding' #186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! Would you be willing to add some test cases of files with different encodings? I can help with the test-case plumbing if that would be helpful.
let l:lines = readfile(a:config_filename) | ||
if &encoding !=? 'utf-8' | ||
" strip BOM | ||
if len(l:lines) > 0 && l:lines[0][:2] ==# "\xEF\xBB\xBF" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the first three bytes always come through as the UTF-8 BOM values? E.g., what happens when the file is UTF-16?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part strips the UTF-8 BOM only when it exists. Otherwise it is kept as is.
(The editorconfig spec says that "EditorConfig files should be UTF-8 encoded", so I don't think we need to care about UTF-16.)
Changing 'encoding' has many side effects and that should be avoided. Strip BOM manually. Fix editorconfig#144 Fix editorconfig#147
`readfile()` doesn't convert the encoding of the file. Convert it manually when 'encoding' is not UTF-8.
fa82fb1
to
29fedf1
Compare
Do we need to add some test cases with a |
@k-takata Good question! I don't think we need to update the main plugin tests, since this problem is specific to this plugin. If you or @h3xx can send me some test cases, I'll figure out where to put them. |
How about creating a copy of https://github.com/editorconfig/editorconfig-plugin-tests/tree/master/test_files with just adding a UTF-8 BOM to |
+1 for this PR. Tested these changes against the issue in my PR #194, and it in fact fixes the issue I described. This PR seems more comprehensive than my change so I closed #194. Summary of issue from #194 (~12 lines)
|
This will also fix #196 |
Merged --- let me know if you experience any issues! |
Changing 'encoding' has many side effects and that should be avoided.
Strip BOM manually.
Fix #144
Fix #145
Fix #147