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

support Ruby 3.4.x #1935

Merged
merged 17 commits into from
Jan 12, 2025
Merged

support Ruby 3.4.x #1935

merged 17 commits into from
Jan 12, 2025

Conversation

takahashim
Copy link
Collaborator

Ruby 3.4対応です。
素直に対応するでもあんまり変わらないかも? ということでもろもろ対応中です。

関連

@takahashim takahashim marked this pull request as ready for review December 31, 2024 16:41
Copy link
Owner

@kmuto kmuto left a comment

Choose a reason for hiding this comment

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

たすかります!!

Comment on lines -97 to +99
EnforcedStyle: never
Enabled: true
Exclude:
- 'samples/**/*'
Copy link
Collaborator Author

@takahashim takahashim Jan 3, 2025

Choose a reason for hiding this comment

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

samples以下のマジコメはどうでもいいっしょ…ということで放置しています

Comment on lines -398 to +399
res = ''
res = +''
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

一部破壊的に文字列を使っているところでは、最初の初期化時に+をつけてmutableにしています

Comment on lines -44 to +46
@package_attrs << %Q( prefix="#{prefixes_str}")
@package_attrs = %Q( prefix="#{prefixes_str}")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

再代入は問題ない(既存文字列を破壊するわけではないため)ので、初期化直後に破壊的追加を行っている箇所については再代入に変更しているところもあります

Comment on lines -171 to +173
title = ReVIEW::I18n.t('part', part.number)
title = +ReVIEW::I18n.t('part', part.number)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+''等ではなく、既存のimmutableな文字列をmutableにするために+を追加しているところもあります

Comment on lines -77 to +86
texsrc = default_imgmath_preamble
if @config['imgmath_options']['preamble_file'] && File.readable?(@config['imgmath_options']['preamble_file'])
texsrc = File.read(@config['imgmath_options']['preamble_file'])
end
texsrc_pre =
if @config['imgmath_options']['preamble_file'] && File.readable?(@config['imgmath_options']['preamble_file'])
File.read(@config['imgmath_options']['preamble_file'])
else
default_imgmath_preamble
end

texsrc << <<-EOB
texsrc = <<-EOB
#{texsrc_pre}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

新しい文字列を作る際に、既存のimmutableな文字列を埋め込むことはできるので、破壊的追加を行っていたところに別の(immutableな)文字列用変数を追加しているところもあります

Comment on lines -35 to +37
str << "-#{@counter[i]}"
str = "#{str}-#{@counter[i]}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

これも破壊的追加の代わりに、immutableな文字列を埋め込んで都度新しいimmutableな文字列を作ることで回避したものです。
基本的に<<は全部この手の変更で置き換えられそうですが、それはそれで面倒かつコードが読みやすくならなさそうなので、この書き方はたまにしか使ってません。

Comment on lines -66 to +76
prefix = if @chapter.is_a?(ReVIEW::Book::Part)
I18n.t('part_short', @chapter.number)
else
@chapter.format_number(false)
end
prefix = []
prefix << if @chapter.is_a?(ReVIEW::Book::Part)
I18n.t('part_short', @chapter.number)
else
@chapter.format_number(false)
end
0.upto(level - 2) { |i| prefix << ".#{@counter[i]}" }
prefix << I18n.t('chapter_postfix')
prefix
prefix.join
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

「文字列はimmutableだけど配列は別にimmutableにならない」かつ「<<は文字列にも配列にも使える」というのを利用して、<<はそのままにしつつ最初の初期化時に文字列から配列に変更し、最後に.joinして文字列化する、というのも使っています

Comment on lines -918 to +921
@log_io.string = ''
@log_io.rewind
@log_io.truncate(0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

これは+''でも良かったのですが、毎回新しい文字列オブジェクトを作るのはあんまり効率良くないよな…と思い返して、文字列は再利用する形でカーソルを先頭に戻す+既存のデータを初期化するように変更しています。
まあテストなので落ちてなければ大丈夫かな…というくらいの温度感です。

Comment on lines -33 to +35
assert_match(/SUCCESS/, e)
assert_match(/SUCCESS|INFO/, e) # XXX TTY::Logger should be fixed
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

これはTTY::Loggerの問題のせいなので、そちらが直るまではいったんこれでしのごうかと思っています。

Comment on lines -9 to +11
li = LineInput.new(io)
li = ReVIEW::LineInput.new(io)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

これは単独のテストで動かすと落ちるようだったので修正しています(まあ確かに落ちるというか、クラス名ちゃんと指定するべきだよね…とは思いました)。

Comment on lines -64 to +68
data = ''
li.each { |l| data << l }
assert_equal content, data
data = []
li.each { |l| data << l } # rubocop:disable Style/MapIntoArray
assert_equal content, data.join
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

これも文字列から配列に変えたものです。
rubocopが「eachは使わない方が…」とか言ってくるのですが、こっちはeachのテストをするために使っているのでコメントで抑止しています。

Comment on lines -184 to +186
assert_match(/undefined local variable or method `not_existed_method'/, error_msg)
assert_match(/undefined local variable or method ('|`)not_existed_method'/, error_msg)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

これはRuby 3.4でエラーメッセージのフォーマットが変わったやつへの対応です。
以前のRubyでも動くようにどっちでも許すようにしています。

@takahashim
Copy link
Collaborator Author

主な変更点については一通りコメントしておきました

@takahashim
Copy link
Collaborator Author

TTY::Loggerは動きがないので、このPRでマージしておきますね

@takahashim takahashim merged commit 84ffc9e into master Jan 12, 2025
21 checks passed
@takahashim takahashim deleted the fix-ruby34 branch January 12, 2025 02:49
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