Skip to content
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

Add the missing magic header # encoding: utf-8 #3723

Closed
wants to merge 1 commit into from

Conversation

purbon
Copy link
Contributor

@purbon purbon commented Aug 12, 2015

… so all internal strings are UTF-8 in Ruby < 2.0, this has been found thanks to the work on #3718, we might like to work on internal check for this, probably with the inclusion of rubocop or other kind of changes.

@purbon
Copy link
Contributor Author

purbon commented Aug 12, 2015

@jsvd or @jordansissel are you willing to get an eye here, and let me know if this is good to be reviewed ... this is the follow up from #3718 where we discover some missing utf-8 magic headers.

@jsvd
Copy link
Member

jsvd commented Aug 13, 2015

@purbon do you think it's worth modifying the config_ast.rb generator so the file is created with the encoding line on top?

@purbon
Copy link
Contributor Author

purbon commented Aug 13, 2015

We might do it, not sure how to, but I can investigate 🐼

@purbon
Copy link
Contributor Author

purbon commented Aug 14, 2015

@jsvd I was looking at the config_ast.rb and no clue how to make this happen.

@jsvd
Copy link
Member

jsvd commented Aug 14, 2015

The only .rb file without encoding is the generated grammar file lib/logstash/config/grammar.rb.
Making treetop generate the encoding header would require monkeypatching treetop or doing a post prepend on the file.
Unless someone sees a big reason to take on this job, we can merge this PR as it is.

This pending question aside, LGTM

purbon pushed a commit that referenced this pull request Aug 17, 2015
@elasticsearch-bot
Copy link

Merged sucessfully into master 1.5 1.6!

purbon pushed a commit that referenced this pull request Aug 17, 2015
@purbon purbon closed this in 028d764 Aug 17, 2015
@purbon purbon deleted the fix/update_utf8 branch May 10, 2016 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants