Get tests passing with frozen-string-literals enabled.

[pat]

These changes ensure that all string literals can be frozen (as per the optional feature in MRI 2.3 and onwards). I would recommend adding the following to your .travis.yml file to ensure regressions aren't introduced:

before_script:
- if (ruby -e "exit RUBY_VERSION.to_f >= 2.4"); then export RUBYOPT="--enable-frozen-string-literal"; fi; echo $RUBYOPT

This will add the flag when the tests are run on MRI 2.4 or newer (while the feature was introduced in 2.3, it doesn't seem to work reliably until 2.4). Please note: this does require the latest commits for RSpec (particularly: rspec-core and rspec-support).

[imanel]

Hi Pat. Thanks for your contribution. I've checked your code and have couple suggestions:

• Feel free to add mentioned before_script to .travis.yml file, with one change - since we need to test both standard behaviour and frozen strings please add 2.4.1 in addition to 2.4.0 and run frozen string check only on 2.4.0. That way we'll have both options covered, and when 3.0 will be released we can remove this hack.
• There's no reason for so much usage of dup in code. Could you replace it with String.new?
• Looking at changes in tests we're modifying provided data at some point, which should not be the case. Could you remove dup from tests, and check where we're trying to modify input? Great catch btw - it's potential source of problems.

[pat]

Thanks for the feedback Bernard. I've done the following:

• Switched from dup to String.new. The latter had been my default, but in making similar patches to Rails and other libraries, other devs had expressed a preference for dup, hence my use of that.
• Removed all changes to the tests - instead, I'm now using force_encoding on a copy of the strings in Websocket::Frame::Data#convert_args - this is where inputs were being modified.
• Added the before_script for Travis that operates only on MRI 2.4.0. Given the change isn't destructive, I'm not sure you need to test with the flag disabled as well, but happy to go with your call on that :)

[pat]

Oh, and just to clarify from the line of code you highlighted: to_s returns itself, not a copy, if the object is already a string. Hence, the frozen strings were being passed through all the way to the force_encoding call.

[pat]

The Rubocop failures seem to be due to the version change (your last green build was using 0.47.1, the latest is 0.49.1) - the default preferences have changed. Would you prefer to lock the version dependency in the gemspec to 0.47.1, or should I just leave all this to you? :)

Oh, and the parser gem (a dependency of Rubocop) doesn't work with frozen string literals enabled, so I'll look into submitting a patch for that as well.

[pat]

The parser patch is now submitted - I can add it to the Gemfile if you like? I don't wish to presume that you're fine with relying on a fork though :)

[imanel]

What a busy day you had! Thanks for hard work :)
As for questions:

• dup vs String.new is currently part of hot discussion. I personally prefer later, but final judge will be community of Rubocop 😉
• I always prefer new defaults of Rubocop over my old habits - it's easier to adjust for one person than whole community. You can lock 0.47.1 for now, and I will update it after merging your PR (I would prefer to merge green builds), or you can update it and add ignores to rubocop.yml
• You can add parser fork to Gemfile - we need green build, and when your PR gets merged I will clean it up. Please add comment about why it's there (together with rspec from branch).

[pat]

Huh. Parser generates some of its Ruby files via Ragel, and those generated files aren't kept in source control. Makes sense, but means we can't rely on a git repo (it only worked in my testing because I'd generated the files locally while running its own test suite). That's annoying :|

[imanel]

Yes, it is. We need to wait for merging your PR for parser, let's continue after that.

[pat]

Coming back to this PR after several months with some evolved thoughts:

These days I'm actually preferring the pragma comment # frozen_string_literal: true at the top of each file, rather than the before_script option I've added to the travis configuration. It still has the same result: raising errors in MRI 2.3+ by treating string literals as frozen by default.

The catch is that it becomes something that needs to be remembered every time new files are introduced into the project - but removes the need for dependencies to be completely frozen-string-literal-friendly. In this project, given you're using Rubocop then you can have that enforce the pragma comment for future files.

I'm happy to alter this PR to include the pragma comments and remove the Travis before_script option, if that sounds like a way you'd like to move forward?

[imanel]

Sure, I'm kind of surprised that Rubocop didn't forced those. Please alter this PR and after passing tests (and fixing conflicts) I will merge it.

[pat]

Just force-pushed a new version of the branch based on your latest master. Makes for a better commit history, plus easier to merge :)

One thing I've not done is update Rubocop. The latest is 0.52.1, and that seems to be what CodeClimate is now using (I noticed you'd locked the version for that reason). However, updating introduces a whole bunch of Rubocop failures, and while I'm happy to fix them if you'd like, I think that'd be better as a separate PR.

I'm guessing the version you're currently on doesn't enforce the pragma comments (whereas I've been using 0.52.1 on my own projects, and I know that it does).

[imanel]

Awesome, thanks for your work! I will bump Rubocop version in separate PR, thanks for mentioning it.