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

ファイル存在チェックの強化 #1727

Merged
merged 10 commits into from
Sep 5, 2021
Merged

ファイル存在チェックの強化 #1727

merged 10 commits into from
Sep 5, 2021

Conversation

kmuto
Copy link
Owner

@kmuto kmuto commented Sep 3, 2021

#1726 のEPUB coverimage対応と、PDFMakerの外部ファイル取り込み部のチェック強化です。

  • EPUBMaker: coverimage指定時に実ファイルがないときにバックトレースではなくエラーメッセージで終了させる(ApplicationErrorを明示raiseするようにしました)。
  • PDFMaker: coverimage指定時に実ファイルがないときにコンパイル前に即エラーするようにしました。
  • PDFMaker: titlefile, creditfile, profileなどで外部ファイルをインポートするよう設定しているときにファイルの存在チェックをするようにしました(pdfmaker.rb#make_custom_page 内)。

実ファイルが絡むのでテストを作りづらい…。

@kmuto kmuto requested a review from takahashim September 3, 2021 07:12
@@ -471,6 +479,8 @@ def latex_config
result << "%% END: config-local.tex.erb\n"
end
result
rescue StandardError => e
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
rescue StandardError => e
rescue ApplicationError => e

ReVIEW::ConfigError等を拾うのであれば ApplicationError でできます(何でもStandardErrorで握りつぶしてしまうのは良くなさそうなので)

Copy link
Owner Author

Choose a reason for hiding this comment

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

ERBのbindでエラーが出たときにSyntaxError, ZeroDivisionErrorなど予想がつかない生の例外がきてしまうんですよね。ApplicationErrorで拾えなくて困っています。

@kmuto
Copy link
Owner Author

kmuto commented Sep 4, 2021

StandardErrorにしていた理由としては、
たとえばlayouts/config-local.tex.erbを以下のように用意したとして

<%= 1/0 %>

ApplicationErrorのみの捕捉だと

Traceback (most recent call last):
	17: from /home/kmuto/review/bin/review-pdfmaker:14:in `<main>'
	16: from /home/kmuto/review/lib/review/pdfmaker.rb:93:in `execute'
	15: from /home/kmuto/review/lib/review/pdfmaker.rb:147:in `execute'
	14: from /home/kmuto/review/lib/review/pdfmaker.rb:287:in `generate_pdf'
	13: from /home/kmuto/review/lib/review/pdfmaker.rb:194:in `build_pdf'
	12: from /home/kmuto/review/lib/review/pdfmaker.rb:498:in `template_content'
	11: from /home/kmuto/review/lib/review/template.rb:15:in `generate'
	10: from /home/kmuto/review/lib/review/template.rb:30:in `result'
	 9: from /usr/lib/ruby/2.5.0/erb.rb:876:in `result'
	 8: from /usr/lib/ruby/2.5.0/erb.rb:876:in `eval'
	 7: from (erb):2:in `template_content'
	 6: from /home/kmuto/review/lib/review/pdfmaker.rb:478:in `latex_config'
	 5: from /home/kmuto/review/lib/review/template.rb:15:in `generate'
	 4: from /home/kmuto/review/lib/review/template.rb:30:in `result'
	 3: from /usr/lib/ruby/2.5.0/erb.rb:876:in `result'
	 2: from /usr/lib/ruby/2.5.0/erb.rb:876:in `eval'
	 1: from (erb):1:in `latex_config'
(erb):1:in `/': divided by 0 (ZeroDivisionError)

と生の例外がきてバックトレースが出てしまいます。バックトレースを出さずに妥当なエラーメッセージだけを表示したいとなると全捕捉するしかなさそうで困っているのでした。

例は極端ですが、String期待のつもりがnilでエラーを起こすみたいなパターンはそれなりにありそうです。

@takahashim
Copy link
Collaborator

うーん、テンプレートまで手を入れている場合、むしろバックトレース等含めてエラー出してくれた方がテンプレート編集のデバッグがやりやすくないですか…?(どこまで手を入れるかにもよりますが)

*.re を編集してる程度では出したくない、というのはあります。

@kmuto
Copy link
Owner Author

kmuto commented Sep 4, 2021

上記のトレースだと7より上のものが無意味なのが気にいらないんですよねぇ…。まぁlayouts.tex.erbやconfig-local.erbを持ってる時点で覚悟せよとすべきか。ただconfig.erbレベルでももしかしたらconfig.ymlによってはERBエラー起こせるかも。

@takahashim
Copy link
Collaborator

そうですね、config.yml の設定でバックトレースが出るのもうれしくない(役に立たない)ので、config関連の値チェックを適宜追加するのは良いかなと思います。

@takahashim
Copy link
Collaborator

*.re*.ymlではバックトレースや内部のエラーメッセージは出さないけど、*.rbとか*.erbは別扱い、というところで区別するのがいいですかねえ。

@kmuto
Copy link
Owner Author

kmuto commented Sep 4, 2021

今のTemplateからだとApplicationErrorをスローするものがないのでrescue ApplicationErrorはだめそうでした。
とはいえRe:VIEWとしてエラーとしては把握しているよということを示さずにクラッシュするというのは気持ちが悪いので、full_message のほうを使ってみました。

rake pdf
 ...
 ERROR   template or configuration error: Traceback (most recent call last):
	17: from /home/kmuto/review/bin/review-pdfmaker:14:in `<main>'
	16: from /home/kmuto/review/lib/review/pdfmaker.rb:93:in `execute'
	15: from /home/kmuto/review/lib/review/pdfmaker.rb:147:in `execute'
	14: from /home/kmuto/review/lib/review/pdfmaker.rb:287:in `generate_pdf'
	13: from /home/kmuto/review/lib/review/pdfmaker.rb:194:in `build_pdf'
	12: from /home/kmuto/review/lib/review/pdfmaker.rb:496:in `template_content'
	11: from /home/kmuto/review/lib/review/template.rb:15:in `generate'
	10: from /home/kmuto/review/lib/review/template.rb:30:in `result'
	 9: from /usr/lib/ruby/2.5.0/erb.rb:876:in `result'
	 8: from /usr/lib/ruby/2.5.0/erb.rb:876:in `eval'
	 7: from (erb):2:in `template_content'
	 6: from /home/kmuto/review/lib/review/pdfmaker.rb:478:in `latex_config'
	 5: from /home/kmuto/review/lib/review/template.rb:15:in `generate'
	 4: from /home/kmuto/review/lib/review/template.rb:30:in `result'
	 3: from /usr/lib/ruby/2.5.0/erb.rb:876:in `result'
	 2: from /usr/lib/ruby/2.5.0/erb.rb:876:in `eval'
	 1: from (erb):1:in `latex_config'
(erb):1:in `/': divided by 0 (ZeroDivisionError)

これだといかがでしょう。

@takahashim
Copy link
Collaborator

なるほどです、このあたりが落とし所でしょうか。

@kmuto kmuto merged commit 1c9fff4 into master Sep 5, 2021
@kmuto kmuto deleted the coverimage-check branch September 5, 2021 01:48
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