search

thoughtbot / shoulda-matchers

Simple one-liner tests for common Rails functionality
https://matchers.shoulda.io
MIT License
3.5k stars 911 forks source link

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

Closed amalrik closed 7 months ago

amalrik commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

vsppedro commented 8 months ago

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?

https://github.com/thoughtbot/shoulda-matchers/blob/d611911f0175fe6712025dac9f150b0a741d43c0/.github/workflows/ci.yml#L64

https://github.com/thoughtbot/shoulda-matchers/blob/d611911f0175fe6712025dac9f150b0a741d43c0/.github/workflows/ci.yml#L66

amalrik commented 8 months ago

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?

https://github.com/thoughtbot/shoulda-matchers/blob/d611911f0175fe6712025dac9f150b0a741d43c0/.github/workflows/ci.yml#L64

https://github.com/thoughtbot/shoulda-matchers/blob/d611911f0175fe6712025dac9f150b0a741d43c0/.github/workflows/ci.yml#L66

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 commented 8 months ago

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 commented 8 months ago

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