Fix compatibility with frozen-string-literal and newer rubies #80
Conversation
Also setup CI Co-Authored-By: Pat Allan <pat@freelancing-gods.com>
|
👋 @kyrylo @rf- @ConradIrwin sorry for the cold ping, but could any of you review and hopefully cut a new release? For context: https://twitter.com/banisterfiend/status/1779810797251895464 |
| @@ -104,15 +104,15 @@ def extract_first_expression(lines, consume=0, &block) | |||
| # @param [Array<String>] lines | |||
| # @return [String] | |||
| def extract_last_comment(lines) | |||
| buffer = "" | |||
| buffer = +"" | |||
There was a problem hiding this comment.
Is this the same as String.new("")?
In case it is, maybe we could use it instead? It's less cryptic IMO.
There was a problem hiding this comment.
It's not quite the same. +@ only dup the string if it's not already mutable. String.new("") will always create a copy.
But it doesn't make a big difference, so I don't mind changing it. But then "".dup might be even more readable.
There was a problem hiding this comment.
Provided +"" is valid Ruby going back several versions (which I think it is - and the CI runs should confirm that), then I personally would prefer keeping it. Yes, right now it's not common syntax, but taking a step towards normalising this in Ruby code feels smart to me - it creates a single mutable string, which is exactly what's required.
String.new("") and "".dup are, as @casperisfine has noted, both duplicating strings - yes, it's a small thing, but I feel like it's not the best habit to lean into from a performance perspective.
There was a problem hiding this comment.
Ok, thanks for the clarification. This is what I found:
https://github.com/fastruby/fast-ruby?tab=readme-ov-file#string
String#dup vs String#+ code
Note that String.new is not the same as the options compared, since it is always ASCII-8BIT encoded instead of the script encoding (usually UTF-8).
$ ruby -v code/string/dup-vs-unary-plus.rb
ruby 2.4.3p205 (2017-12-14 revision 61247) [x86_64-darwin17]Calculating -------------------------------------
String#+@ 7.697M (± 1.4%) i/s - 38.634M in 5.020313s
String#dup 3.566M (± 1.0%) i/s - 17.860M in 5.008377sComparison:
String#+@: 7697108.3 i/s
String#dup: 3566485.7 i/s - 2.16x slower
Let's stick with +""
|
This is looking good, @casperisfine, thank you for the PR! I only have a minor question, and then this is good to be merged and released. |
|
Thank you! |
Also setup CI
Fixes: #47
cc @jhawthorn, @pat.
Also @banister if you are looking for maintainers for this gem, let us know, as it would be a shame if it broke on a future Ruby version.