-
Notifications
You must be signed in to change notification settings - Fork 117
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
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 thanks a ton!
lib/simplecov-html/version.rb
Outdated
@@ -1,7 +1,7 @@ | |||
module SimpleCov | |||
module Formatter | |||
class HTMLFormatter | |||
VERSION = "0.10.1" | |||
VERSION = "0.10.1".dup | |||
|
|||
def VERSION.to_a |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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! 👍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even better :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, in both repos 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
lib/simplecov-html/version.rb
Outdated
@@ -1,7 +1,7 @@ | |||
module SimpleCov | |||
module Formatter | |||
class HTMLFormatter | |||
VERSION = "0.10.1" | |||
VERSION = "0.10.1".dup | |||
|
|||
def VERSION.to_a |
There was a problem hiding this comment.
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.
Thanks a ton! 🎉 |
0.10.2 2017-08-14 ======== ## Bugfixes * Allow usage with frozen-string-literal-enabled. See [#56](simplecov-ruby/simplecov-html#56) (thanks @pat)
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: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).