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.

Sign up for GitHub

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

Jump to bottom

Assume UTF-8 when detecting Rails #2469

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Assume UTF-8 when detecting Rails #2469

merged 1 commit into from
Aug 20, 2024

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Aug 20, 2024 •
edited
Loading

Motivation

Closes #2463

Implementation

As per suggestion by @kddnewton, we can force-assume UTF-8 when reading the file.

Automated Tests

Undecided on approach. @Earlopain illustrated one way in #2467 but I'm not sure it's worth adding another job.

Edit: Applied a suggestion from @paracycle

Manual Tests

tbc - waiting to hear back from the origina reporters about their LANG setting.

@andyw8 andyw8 added the bugfix This PR will fix an existing bug label Aug 20, 2024
@andyw8 andyw8 force-pushed the andyw8/assume-config-application-rb-is-utf8 branch from df096cf to 40f3b34 Compare August 20, 2024 18:35
andyw8
andyw8 commented Aug 20, 2024
View reviewed changes
test/setup_bundler_test.rb Outdated Show resolved Hide resolved
vinistock
vinistock approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Looks good

test/setup_bundler_test.rb Outdated Show resolved Hide resolved
@vinistock vinistock added the server This pull request should be included in the server gem's release notes label Aug 20, 2024
@vinistock vinistock mentioned this pull request Aug 20, 2024
Closed
@andyw8 andyw8 force-pushed the andyw8/assume-config-application-rb-is-utf8 branch from 716b7b9 to 76e499d Compare August 20, 2024 19:45
@andyw8
Copy link
Contributor Author

andyw8 commented Aug 20, 2024

Updated based on feedback. Still trying to understand the remaining type error, seems the shim isn't being applied?

@andyw8 andyw8 force-pushed the andyw8/assume-config-application-rb-is-utf8 branch from 76e499d to 03788fa Compare August 20, 2024 20:27
@andyw8 andyw8 changed the title WIP: Assume UTF-8 when detecting Rails Assume UTF-8 when detecting Rails Aug 20, 2024
@andyw8 andyw8 force-pushed the andyw8/assume-config-application-rb-is-utf8 branch from 03788fa to 3501be2 Compare August 20, 2024 20:28
@andyw8 andyw8 marked this pull request as ready for review August 20, 2024 20:28
@andyw8 andyw8 requested a review from a team as a code owner August 20, 2024 20:28
@andyw8 andyw8 requested a review from st0012 August 20, 2024 20:28
@andyw8 andyw8 enabled auto-merge (squash) August 20, 2024 20:29
@andyw8 andyw8 mentioned this pull request Aug 20, 2024
Closed
st0012
st0012 approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

I think a shim is not needed, but overall looks good.

sorbet/rbi/shims/encoding.rbi Outdated Show resolved Hide resolved
@andyw8 andyw8 force-pushed the andyw8/assume-config-application-rb-is-utf8 branch from 3501be2 to 0d9dcec Compare August 20, 2024 20:54
@andyw8 andyw8 force-pushed the andyw8/assume-config-application-rb-is-utf8 branch from 0d9dcec to d00d6c0 Compare August 20, 2024 20:56
@andyw8 andyw8 merged commit d9c0158 into main Aug 20, 2024
33 checks passed
@andyw8 andyw8 deleted the andyw8/assume-config-application-rb-is-utf8 branch August 20, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinistock vinistock vinistock approved these changes

@Earlopain Earlopain Earlopain left review comments

@paracycle paracycle paracycle left review comments

@st0012 st0012 st0012 approved these changes

Assignees
No one assigned
Labels
bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

setup_bundler's rails_app? incompatible with non-ascii characters
5 participants
@andyw8 @paracycle @st0012 @Earlopain @vinistock