Improve frozen-string-literals compatibility.

[pat]

Some changes to ensure syntax is happy to run when the --enable-frozen-string-literal flag is in place via RUBYOPT when using MRI 2.4. The tests passed for me locally both before and after these changes.

I realise this is a bit of a surprising patch for a library that's built with MRI 1.9, but I've followed the path of dependencies from rspec to cucumber to here, patching as I go :)

Also: I've opted for "".dup instead of String.new, because the latter has an ASCII encoding, whereas String.new("") respects the argument's encoding (UTF-8 in recent MRI releases). Happy to tweak things if you've a preferred syntax.

[dblock]

Interesting. Before I merge, do you think you can write a failing test for this?

[dblock]

I do think String.new is "better", but YOLO.

[pat]

Can't think of any way to write a test for this thus far… though just to be clear: without my patch, if you run the tests on MRI 2.4.x with RUBYOPT="--enable-frozen-string-literal" there will be failures.

Generally in my patches I've been recommending people add the following to their Travis CI configuration (provided they're also testing against a 2.4 release):

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

But from what I can see, Travis isn't being used for this repo, so that's not so useful here.

[pat]

I'm very happy to switch "".dup to String.new("") if you'd like - it's just String.new() that isn't consistent. I've gotten into the habit of using dup because it's what the Rails core team preferred with my patches there, but I don't have a strong personal preference one way or the other.

[dblock]

You also need to update CHANGELOG, please.

Do you have time to add CI to this project in a separate PR? I inherited syntax as nobody wanted to maintain it, could use some help and this is a good opportunity.

[dblock]

I'm A-OK with "".dup. I'll merge with CHANGELOG and call it a day. Want to help out with a release?

[pat]

Changelog is updated, and about to create another PR with a working Travis configuration.

[dblock]

Now that the Travis-CI config is merged, rebase this and add the configuration w/ a test that will otherwise fail, ie. with --enable-frozen-string-literal?

[pat]

Rebased with RUBYOPT configuration, and the tests on MRI 2.4.1 are green 👍 https://travis-ci.org/dblock/syntax/jobs/248545023 - shall sort out a release later today (if I've got access).

[dblock]

I don't believe this export propagates further actually.

Can we instead add a matrix include config with ruby 2.4.1 that runs this explicitly instead of a conditional? You can set an env key with a value explicitly in travis.

[dblock]

Like this: https://github.com/ruby-grape/grape/blob/master/.travis.yml#L5

[pat]

The export does work (just confirmed with a test branch on my fork: https://travis-ci.org/pat/syntax/jobs/249047431), but will switch over to the other syntax anyway.

[dblock]

Merged. Thank you. Feel free to cut a release.