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

YAML(Psych)の動作互換性保持 #1775

Merged
merged 8 commits into from
Jan 7, 2022
Merged

YAML(Psych)の動作互換性保持 #1775

merged 8 commits into from
Jan 7, 2022

Conversation

kmuto
Copy link
Owner

@kmuto kmuto commented Jan 6, 2022

#1774 の対応です。
YAML.review_load_fileを定義し、safe_load_fileメソッドの存在有無で分岐して妥当なほうを使います。

  • YAML.review_load_file という名前が微妙そうだが、load_file は使ってるのでどうしたものか。
  • YAML.review_load_file の定義場所がextensions/yaml.rb でいいかどうか。

@kmuto kmuto requested a review from takahashim January 6, 2022 05:46
@kmuto
Copy link
Owner Author

kmuto commented Jan 6, 2022

@takahashim
CIだとPsych指定なので判断つかないですが、手元のDebian stable、bundler経由なしでテストおよび実行を通っています。
ruby 3.1のDocker環境でも動作しました。

@takahashim
Copy link
Collaborator

takahashim commented Jan 6, 2022

YAML.review_load_fileについてですが、最近は既存クラスにメソッドを追加するのはあまり好まれない気がするので、ReVIEW::YAMLLoader.safe_load_file というクラスメソッドとして実装する方がいいかも、と思いました。

以下のような実装を想定しています。

module ReVIEW
  class YAMLLoader
    def self.safe_load_file(file)
      if  YAML.respond_to?(:safe_load_file)
        YAML.safe_load_file(file, aliases: true, permitted_classes: [Date])
      else
        File.open(file, 'rt:bom|utf-8') do |f|
          YAML.safe_load(f, filename: file, aliases: true, permitted_classes: [Date])
        end
      end
    end

    def initialize
    end
# (略)

@kmuto
Copy link
Owner Author

kmuto commented Jan 6, 2022

ありがとうございます、Re:VIEWで独自クラス作ったほうがいいのかなと確かに思っていたところです。

Ruby 2.7未満向けだとsafe_loadでArgumentError例外(filename, permitted_classesがない)をさらに捕捉してやればいいのかな。(手元の業務TeX環境がDebian oldstable + Ruby 2.5ベースでした…)

@kmuto
Copy link
Owner Author

kmuto commented Jan 7, 2022

@takahashim
2.4対応がいささか気持ち悪い(ファイルオブジェクト経由のf.readだとダメでFile.readだとOKなのはなんだろう…)ですが、ReVIEW::YAMLLoader側にsafe_load_fileおよびsafe_loadを実装し、全バージョン通るようにしました。

@takahashim
Copy link
Collaborator

む、要は文字列になってないと駄目だった、ということなんですかね>f.readだとダメでFile.readだとOK
コードはこれでよいかと思います。修正ありがとうございました!

@takahashim takahashim merged commit 08d66b8 into master Jan 7, 2022
@takahashim takahashim deleted the psych-compatibility branch January 7, 2022 11:02
@kmuto kmuto mentioned this pull request Jan 7, 2022
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.

None yet

2 participants