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

Handle US_ASCII encoding when parsing rails application config #2467

Closed
wants to merge 1 commit into from

Conversation

Earlopain
Copy link
Contributor

@Earlopain Earlopain commented Aug 20, 2024

Motivation

Connects to #2463

Implementation

Added ascii encoding tests to the CI pipeline. Surprisingly to me, this was the only test that failed (and some multibyte which make less sense for US_ASCII encoding).

Not that knowledgable on this encoding stuff, but I do know that if ruby gets run with proper LANG environment variables, Encoding.default_external will reflect that which then causes File.read and the like to also read files in that encoding.

Could in theory force the encoding by overwriting Encoding.default_external but however the user has set this up, there is probably a reason for it.

Automated Tests

Run the test suite with a encoding different to utf8.

Manual Tests

None, I'm not really sure how ):. I did at least confirm that this doesn't break when the encoding is utf8.

@@ -289,6 +289,7 @@ def rails_app?
application_contents = config.read if config.exist?
return false unless application_contents

application_contents = application_contents.encode(Encoding::UTF_8, Encoding::UTF_8, invalid: :replace)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

force_encoding would do the job as well but this is more robust against whatever else may be present in that file

@Earlopain Earlopain force-pushed the rails-app-us-ascii branch 6 times, most recently from 27fb72d to d0c4d02 Compare August 20, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant