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

Allow library to be used with frozen-string-literals enabled. #56

Merged
merged 2 commits into from
Jun 24, 2017

Conversation

pat
Copy link
Contributor

@pat pat commented Jun 22, 2017

This change ensures 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).

pat added a commit to pat/simplecov that referenced this pull request Jun 22, 2017
For the tests to pass, this requires the use of the latest RSpec commits, along with submitted patches to simplecov-html and test-unit:
* simplecov-ruby/simplecov-html#56
* test-unit/test-unit#149
Copy link
Collaborator

@PragTob PragTob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 thanks a ton!

@@ -1,7 +1,7 @@
module SimpleCov
module Formatter
class HTMLFormatter
VERSION = "0.10.1"
VERSION = "0.10.1".dup

def VERSION.to_a
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, I think I'd rather think the definition of the method on the object itself then going the dup route :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I change things so VERSION returns an object with these methods, plus to_s that returns the full string? Or should VERSION just be a normal string, no monkey-patched methods?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, good question. I think I'd have just added a to_a(string) method as the easiest fix. Design wise I think an object would be best as it could also hold onto the individual parts internally. But it might be a bit of an overkill for this limited use case :D

Imo go with whatever you prefer, both are fine by me :) Thanks a lot! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning towards VERSION just being a standard string, and removing all the helper methods. They're not used anywhere in the code from what I could see, so the only downside is it might break things in other libraries… the odds of that, I would hope, are pretty small.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even better :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, in both repos 👍

Copy link
Collaborator

@PragTob PragTob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@@ -1,7 +1,7 @@
module SimpleCov
module Formatter
class HTMLFormatter
VERSION = "0.10.1"
VERSION = "0.10.1".dup

def VERSION.to_a
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, good question. I think I'd have just added a to_a(string) method as the easiest fix. Design wise I think an object would be best as it could also hold onto the individual parts internally. But it might be a bit of an overkill for this limited use case :D

Imo go with whatever you prefer, both are fine by me :) Thanks a lot! 👍

Also, ensure it's always frozen, no matter which version or configuration of Ruby you're using.
@PragTob
Copy link
Collaborator

PragTob commented Jun 24, 2017

Thanks a ton! 🎉

@PragTob PragTob merged commit 126841d into simplecov-ruby:master Jun 24, 2017
PragTob added a commit that referenced this pull request Jun 24, 2017
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Aug 31, 2017
0.10.2 2017-08-14
========

## Bugfixes

* Allow usage with frozen-string-literal-enabled. See [#56](simplecov-ruby/simplecov-html#56) (thanks @pat)
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.

2 participants