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

Get tests passing with frozen-string-literals enabled. #30

Merged
merged 3 commits into from Jan 2, 2018

Conversation

Projects
None yet
2 participants
@pat
Contributor

pat commented Jun 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@imanel

imanel Jun 21, 2017

Owner

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.
Owner

imanel commented Jun 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@pat

pat Jun 22, 2017

Contributor

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 :)
Contributor

pat commented Jun 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@pat

pat Jun 22, 2017

Contributor

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.

Contributor

pat commented Jun 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@pat

pat Jun 22, 2017

Contributor

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.

Contributor

pat commented Jun 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@pat

pat Jun 22, 2017

Contributor

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 :)

Contributor

pat commented Jun 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@imanel

imanel Jun 22, 2017

Owner

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).
Owner

imanel commented Jun 22, 2017

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 added a commit to pat/websocket-ruby that referenced this pull request Jun 22, 2017

Lock rubocop to 0.47.1 for the moment.
The syntax of this library will need updating for the latest Rubocop version, as discussed in #30 by @imanel.
@pat

This comment has been minimized.

Show comment
Hide comment
@pat

pat Jun 22, 2017

Contributor

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 :|

Contributor

pat commented Jun 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@imanel

imanel Jun 22, 2017

Owner

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

Owner

imanel commented Jun 22, 2017

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

@pat

This comment has been minimized.

Show comment
Hide comment
@pat

pat Jan 2, 2018

Contributor

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?

Contributor

pat commented Jan 2, 2018

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

This comment has been minimized.

Show comment
Hide comment
@imanel

imanel Jan 2, 2018

Owner

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.

Owner

imanel commented Jan 2, 2018

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 added some commits Jan 2, 2018

Update RSpec to 3.7.
The 3.7 releases are frozen-string-literal-friendly.
Update code to be frozen-string-literal-friendly.
In the case of convert_args, let's return new objects, rather than modifying the input directly.
@pat

This comment has been minimized.

Show comment
Hide comment
@pat

pat Jan 2, 2018

Contributor

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).

Contributor

pat commented Jan 2, 2018

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 imanel merged commit ffe03df into imanel:master Jan 2, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
codeclimate 1 fixed issue
Details
@imanel

This comment has been minimized.

Show comment
Hide comment
@imanel

imanel Jan 2, 2018

Owner

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

Owner

imanel commented Jan 2, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment