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

EPUBの差し込み静的ファイルの存在チェック #1670

Merged
merged 2 commits into from
Feb 21, 2021

Conversation

kmuto
Copy link
Owner

@kmuto kmuto commented Feb 17, 2021

EPUBでauthorなど外部ファイルをコピーする前にファイルが存在するかどうかのチェックに甘いところがありました(例外が発生してしまっているものがあった)。
頻出するのでメソッドにまとめています。

テストコードを書くつもりです。

@kmuto kmuto changed the title [WIP] EPUBの差し込み静的ファイルの存在チェック EPUBの差し込み静的ファイルの存在チェック Feb 20, 2021
@kmuto
Copy link
Owner Author

kmuto commented Feb 20, 2021

テスト作成しました。ユニット粒度がちと大きくて壊れたときにわかりにくいかもですが…

@kmuto kmuto requested a review from takahashim February 20, 2021 08:47
@takahashim
Copy link
Collaborator

takahashim commented Feb 21, 2021

ユニットの粒度はたしかに小さい方がいいとは思いました。testの書き方を変えた方がいいでしょうか(最近RSpecばかり書いているので気になるかと言えば気になりました)。

が、それ以前の問題として、epubmaker_instanceとかの作り(実装と呼び出し方)が気になるのですが、それって本質的な問題としてはEPUBMakerのAPIがマズい、という話になるかと思うので、次のメジャーアップデートの時くらいにはなんとかしたいですね…。

ついでにいうと、コピー関連の微妙に実装が違うメソッドが現状いくつもある気がするので、これも整理した方が良さそうとも思いました。

…というところは措いておいて、copy_static_fileの実装については問題ないかと思います。

@takahashim takahashim merged commit f4a0c35 into master Feb 21, 2021
@takahashim takahashim deleted the epub-filecheck branch February 21, 2021 15:46
@kmuto
Copy link
Owner Author

kmuto commented Feb 22, 2021

はい、epubmakerでconfig取れんなこれということに気付いて、accessorするか悩んだんですが、今小手先で元を変えるよりはいずれ全面書き換え(EPUBMakerから一段置くのをやめてもうちょっと近い形で操作)したほうがいいなと思いました。
コピー関連の、たしかにそうですね。

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