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 frozen string literal for ruby 3.4 #719

Merged
merged 1 commit into from
Oct 20, 2024
Merged

Conversation

chaadow
Copy link
Contributor

@chaadow chaadow commented May 24, 2024

After running RUBYOPT='--enable-frozen-string-literal --debug-frozen-string-literal' bin/rails c ( Following this guide )

I had this issue:

/Users/chedli/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/spring-4.2.1/lib/spring/json.rb:367:in `unquote': can't modify frozen String: "", created at /Users/chedli/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/spring-4.2.1/lib/spring/json.rb:367 

I went ahead and updated github actions as well, inspired by this svenfuchs/rails-i18n#1120

@chaadow chaadow force-pushed the patch-1 branch 4 times, most recently from 83ba6c9 to 5797699 Compare June 10, 2024 12:55
@chaadow
Copy link
Contributor Author

chaadow commented Jun 10, 2024

I think I need help, for some reason the vendored JSON library is not called in acceptance tests? and I know for sure it does a string mutation because my application warns ( and crashes because i've monkey patch ruby's warn method to raise an error )

@chaadow chaadow force-pushed the patch-1 branch 4 times, most recently from e322517 to 0a75db3 Compare June 10, 2024 21:16
@tenderlove
Copy link
Member

@chaadow I think the way to fix this would be to run okjson's tests with frozen string literals enabled, then re-embed it in to Spring. It looks like the upstream repo has been archived though, so I'm not sure who is in charge of this code anymore.

@tenderlove
Copy link
Member

FWIW I ran the OKJson tests using your patch, and that fixed all of the errors there so I think this change is fine. Can you also add the "frozen string literal" directive to the top of json.rb? Thanks!

@chaadow chaadow force-pushed the patch-1 branch 3 times, most recently from 02a8a4b to daf36d7 Compare June 18, 2024 00:33
@chaadow
Copy link
Contributor Author

chaadow commented Jun 18, 2024

@tenderlove Done! thanks for the review. I do think this vendored JSON is outdated and can be switched with a more recent implementation.

The CI is failing, but I can't figure out why in the errors displayed, if you have any tips please let me know.

I'll keep trying to investigate when I got more time.

@chaadow
Copy link
Contributor Author

chaadow commented Jun 23, 2024

@tenderlove Based on this ( #713) the PR was merged even though the CI was failing.

and the CI errors are similar to here.

So maybe, if you don't mind, we can merge this PR and repair CI in another PR?

@chaadow chaadow force-pushed the patch-1 branch 2 times, most recently from c1d39e9 to 9680201 Compare June 23, 2024 12:44
@chaadow
Copy link
Contributor Author

chaadow commented Oct 20, 2024

Hi @tenderlove is it possible to get a last review of this PR please? 🙏🏼

@tenderlove tenderlove merged commit 4bdb337 into rails:main Oct 20, 2024
6 of 7 checks passed
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