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

Fix encoding error when trying to show a diff: #898

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Edouard-chin
Copy link
Member

Ref rails/rails#54733

  • Problem

    Copying or creating a file with UTF-8 charatacters would raise an encoding conversion error when:

    1. There is a collision and the user wants to see a diff.
    2. The application has set its Encoding.default_internal to a non-nil value.

    Context

    In Rails applications, the Encoding.default_internal is set to UTF-8. The Encoding.default_external will be used to transcode a string because the default_internal is not nil. So doing this would raise a Encoding::UndefinedConversionError

    content = "\xE2\x80\x99".force_encoding(Encoding::ASCII_8_BIT)
    File.write("my_file.rb", content)

    In Thor's case, the encoding of the source file will always be ASCII_8_BIT because we are reading the file in binary mode:

    content = File.binread(source)

- ### Problem

  Copying or creating a file with UTF-8 charatacters would raise an
  encoding error when:

  1) There is a collision and the user wants to see a diff.
  2) The application has set its `Encoding.default_internal` to a
  non-nil value.

  ### Context

  In Rails applications, the `Encoding.default_internal` is set to
  UTF-8. The `Encoding.default_external` will be used to transcode
  a string because the `default_internal` is not nil. So doing
  this would raise a `Encoding::UndefinedConversionError`

  ```ruby
    content = "\xE2\x80\x99".force_encoding(Encoding::ASCII_8_BIT)
    File.write("my_file.rb", content)
  ```
Copy link
Member

@skipkayhil skipkayhil left a comment

Choose a reason for hiding this comment

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

Love simple changes, makes sense to me 👍

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.

2 participants