remove frozen string literal setting from the plugin

[rstruber]

This avoids issues with string modification in scripts that do not create objects from the strings.

Example:

Check failed to run: can't modify frozen String, ["/opt/sensu/embedded/lib/ruby/gems/2.4.0/gems/mixlib-cli-1.7.0/lib/mixlib/cli.rb:315:in `build_option_arguments'", "/opt/sensu/embedded/lib/ruby/gems/2.4.0/gems/mixlib-cli-1.7.0/lib/mixlib/cli.rb:269:in `block (2 levels) in opt_parser'", "/opt/sensu/embedded/lib/ruby/gems/2.4.0/gems/mixlib-cli-1.7.0/lib/mixlib/cli.rb:268:in `each'", "/opt/sensu/embedded/lib/ruby/gems/2.4.0/gems/mixlib-cli-1.7.0/lib/mixlib/cli.rb:268:in `block in opt_parser'", "/opt/sensu/embedded/lib/ruby/2.4.0/optparse.rb:1062:in `initialize'", "/opt/sensu/embedded/lib/ruby/gems/2.4.0/gems/mixlib-cli-1.7.0/lib/mixlib/cli.rb:263:in `new'", "/opt/sensu/embedded/lib/ruby/gems/2.4.0/gems/mixlib-cli-1.7.0/lib/mixlib/cli.rb:263:in `opt_parser'", "/opt/sensu/embedded/lib/ruby/gems/2.4.0/gems/mixlib-cli-1.7.0/lib/mixlib/cli.rb:230:in `parse_options'", "/opt/sensu/embedded/lib/ruby/gems/2.4.0/gems/sensu-plugin-2.0.1/lib/sensu-plugin/cli.rb:13:in `initialize'", "/opt/sensu/embedded/lib/ruby/gems/2.4.0/gems/sensu-plugin-2.0.1/lib/sensu-plugin/cli.rb:57:in `new'", "/opt/sensu/embedded/lib/ruby/gems/2.4.0/gems/sensu-plugin-2.0.1/lib/sensu-plugin/cli.rb:57:in `block in <class:CLI>'"]

In bin/check-rabbitmq-node-usage.rb

Pull Request Checklist

Is this in reference to an existing issue?

General

• Update Changelog following the conventions laid out here

• Update README with any necessary configuration snippets

• Binstubs are created if needed

• RuboCop passes

• Existing tests pass

New Plugins

• Tests

• Add the plugin to the README

• Does it have a complete header as outlined here

Purpose

Known Compatibility Issues

[majormoses]

@rstruber sorry I think I was unclear before with any of the ones you are having issues with we could disable it but I was under the impression many of them worked. I have looked up solutions for most of the scenarios which can be worked around using:

• msg = String.new
• msg =+'' having the +'' tells ruby to unfreeze the strings. This was introduced in ruby 2.3 which ruby 2.2 is EOL so I am more than happy to make it a breaking change to drop ruby > 2.3 support.
• .dup (what you did in your last PR)
• msg = [] by making it an array we are sidestepping the issue all together

There might be scenarios where it is still too painful to try to get it to work but I think we should put in the effort where we can. As of ruby 3.0 (which is certainly a while off) it will no longer work to disable them

[majormoses]

@eheydrick what are your thoughts?

[rstruber]

I probably got a little over-zealous. Nothing wrong with paring it down. I would lean toward using the .dup method as that seems the most obvious way of showing what you're doing and future proof as well.

[majormoses]

Sorry for the lack of response, this got lost in the sea of notifications I get get from github.

I probably got a little over-zealous. Nothing wrong with paring it down.

No problem, I appreciate your help.

My "quick" thoughts

I have yet to really see much consensus in the community as to the "ideal" way to handle it. I am leaning more towards String.new but ''.dup is equally valid. Supposedly the unary operator +'' is faster and looks close to the old behavior but I find this not very obvious and requires people to know more about ruby then I think we should shoot for. I'd like to see some folks comment on this before we run around changing all of them to only discover a week later that we want it another way. If you want we can remove them on the ones that are currently broken and we can figure out what people want to see overall. I think we need to create a meta issue in https://github.com/sensu-plugins/community to discuss this issue as its scope goes beyond the rabbitmq plugin.

Some additional thoughts

First off I think this discussion should move to an issue in https://github.com/sensu-plugins/community as its not really specific to this plugin on how we should address it going forward across all the plugins.

I would lean toward using the .dup method as that seems the most obvious way of showing what you're doing and future proof as well.

I think that is what I am seeing in a few places. I think that now I can't be lazy and just always init vars as some_var = '' I find myself leaning towards the more verbose method of some_var = String.new('its_val') for dynamic strings thus leaving some_var = 'some_val' should be treated as frozen strings. I find that this makes it clear as to if we expect the strings value to change or not. When specifying some_var = String.new('some_val') or some_var = String.new it instructs it to set frozen to false on the string object. It results in the creation 1 vs 2 objects but I doubt that makes any real performance impact in our use cases. According to this rubocop rule .dup may not be the most performant solution but I would guess that in the context of small plugins have little bearing on overall performance one way or another.

I think specifically in the case of msg since its always initialized as an empty string I think we should use String.new for this as it is more clear that rather than creating a string object and duplicating that object (which happens to unfrozen) we should just initialize it as a non frozen string in the first place. In the case where we are setting something (such as a default option) I don't really have much of a preference of a = 'foo'.dup vs a = String.new('foo') though for consistency sake I tend to lead towards String.new.

One advantage of using the Unary operator (other than supposedly faster) is encoding. Unless specified it will default to ASCII-8BIT. +'' will use whatever the script is encoded with, which defaults to UTF-8 in modern ruby.

[majormoses]

any chance you plan on coming back to this and getting this over the line?

[rstruber]

I have intentions to. I guess I thought we left off in a place where you were waiting to make a decision. I'll try to pare this down and move it along.

[majormoses]

I wanted to hear your thoughts on what I said, ideally these decisions should be made with community input not just my own personal opinions.