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

make the library work with globally enabled frozen-string-literals #1598

Merged

Conversation

amalrik
Copy link
Contributor

@amalrik amalrik commented Dec 24, 2023

fixes #1563
EDIT:after some more verification I noticed we need more changes in order to make the tests pass with the frozen-string option globally enabled. LMK if you guys think this is a useful contribution to project.

@amalrik amalrik marked this pull request as draft December 25, 2023 16:01
@amalrik
Copy link
Contributor Author

amalrik commented Dec 25, 2023

in order to run the tests and see the error you have to run them this way

RUBYOPT='--enable=frozen-string-literal' bundle exec appraisal rails_7_1 rspec spec/

and ofc you can replace rails_7_1 for the other supported appraisals

@vsppedro
Copy link
Collaborator

I believe it's a positive step to take. If everyone is in agreement, let's proceed with updating the CI to run with this configuration: RUBYOPT='--enable=frozen-string-literal'

@amalrik
Copy link
Contributor Author

amalrik commented Dec 27, 2023

Thanks for the feedback @vsppedro I will put some hours of work on this issue and update this pull request as I progress

@amalrik amalrik marked this pull request as ready for review December 28, 2023 18:58
@vsppedro
Copy link
Collaborator

vsppedro commented Jan 3, 2024

Nice work, @amalrik. I think we need to add RUBYOPT='--enable=frozen-string-literal' to the CI configuration to be sure that is working. What do you think?

run: bundle exec rake spec:unit --trace

run: bundle exec rake spec:acceptance --trace

@amalrik
Copy link
Contributor Author

amalrik commented Jan 3, 2024

Nice work, @amalrik. I think we need to add RUBYOPT='--enable=frozen-string-literal' to the CI configuration to be sure that is working. What do you think?

run: bundle exec rake spec:unit --trace

run: bundle exec rake spec:acceptance --trace

tks man I agree.
I just added the option on CI but let me know if you'd rather to run tests with and without the option. I think maybe its not necessary but LMK your opinion and if I should squash commits as well.

@vsppedro
Copy link
Collaborator

vsppedro commented Jan 3, 2024

To be honest, I'm not a fan of it, but I think it's a good thing to have to ensure that we aren't introducing anything that could inadvertently alter a string and potentially disrupt this feature.

Let me check how other gems approach this.

@amalrik
Copy link
Contributor Author

amalrik commented Jan 10, 2024

hey @vsppedro could you check this?
LMK if I can help you moving this forward

Copy link
Collaborator

@vsppedro vsppedro left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry for taking so long. Please, squash the commits.

more refacts to make tests pass with frozen strings

fix linter errors

clean up some code

enable frozen string literals on CI
@amalrik amalrik force-pushed the fix-frozen-string-literals-enabled-globally branch from e8bf776 to b762994 Compare January 11, 2024 14:13
Copy link
Collaborator

@matsales28 matsales28 left a comment

Choose a reason for hiding this comment

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

LGTM!

@vsppedro vsppedro merged commit 367500d into thoughtbot:main Jan 12, 2024
15 checks passed
@amalrik amalrik deleted the fix-frozen-string-literals-enabled-globally branch January 22, 2024 18:58
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.

The library doesn't work with frozen-string-literals enabled globally.
3 participants