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

Support frozen strings with REXML #728

Merged
merged 6 commits into from
Jan 13, 2025

Conversation

frederikspang
Copy link
Contributor

Tests are passing with export RUBYOPT=--enable-frozen-string-literal

@johnnyshields
Copy link
Collaborator

@frederikspang I am pretty sure this is fixed on v2 already, but @pitbulk it might make sense to fix this on master anyway as it will is needed for Ruby 3.4

@frederikspang
Copy link
Contributor Author

@johnnyshields I've adjusted some stuff from review. UUID_PREFIX is used as a constant, and .replace with Utils.set_prefix, which is not working out great :). I've replaced it with a variable. Should we deprecate or remove the (now unused) constant?

@johnnyshields
Copy link
Collaborator

@frederikspang yikes wow we were calling replace on a constant 🤦‍♂️ . The way you have it now with @@prefix is fine.

@pitbulk
Copy link
Collaborator

pitbulk commented Jan 10, 2025

Changes are not compatible with ruby 2.X versions.

@johnnyshields, for this, the v2 should be used instead. we should address the UUID_PREFIX on that brach.

@johnnyshields
Copy link
Collaborator

johnnyshields commented Jan 11, 2025

@pitbulk frozen string stuff is handled already on 2.x. This is a "backport" in case we want it on 1.x branch, but the alternative here would be to just close this PR and tell users they must use 2.x for Ruby 3.4+. Which is reasonable in my opinion.

@johnnyshields
Copy link
Collaborator

Mutable constant addressed here: #735

@frederikspang
Copy link
Contributor Author

@johnnyshields Frozen strings is not exclusive for Ruby 3.4 - It's just moving towards default frozen, with "chilled string".

I think that not supporting frozen strings for < 3.4 would be a mistake, when the backport is somewhat simple for 1.x branch.

@frederikspang
Copy link
Contributor Author

Specs may have been solved for older ruby versions, with using .dup instead of +"" for unfreezing. Could you rerun specs?

@johnnyshields
Copy link
Collaborator

It's @pitbulk 's call whether he wants to release for 1.x branch.

@johnnyshields
Copy link
Collaborator

I don't think I can run the tests--@pitbulk can you activate?

@pitbulk
Copy link
Collaborator

pitbulk commented Jan 13, 2025

@frederikspang, it seems for ruby >= 3.4.0 we need to add mutex_m gem on the gemspec, can you add it?

/opt/hostedtoolcache/Ruby/3.4.1/x64/lib/ruby/gems/3.4.0/gems/minitest-5.18.1/lib/minitest.rb:3: warning: mutex_m was loaded from the standard library, but is not part of the default gems starting from Ruby 3.4.0.
You can add mutex_m to your Gemfile or gemspec to silence this warning.
Stopped processing SimpleCov as a previous error not related to SimpleCov has been detected
/opt/hostedtoolcache/Ruby/3.4.1/x[6](https://github.com/SAML-Toolkits/ruby-saml/actions/runs/12715610842/job/35546871648?pr=728#step:5:7)4/lib/ruby/3.4.0/bundled_gems.rb:82:in 'Kernel.require': cannot load such file -- mutex_m (LoadError)

@frederikspang
Copy link
Contributor Author

@pitbulk Got it :)

@pitbulk pitbulk merged commit 66893b5 into SAML-Toolkits:master Jan 13, 2025
36 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.

3 participants