Amon icon indicating copy to clipboard operation
Amon copied to clipboard

is config read only?

Open nihen opened this issue 12 years ago • 2 comments

https://github.com/tokuhirom/Amon/blob/master/lib/Amon2/Setup/Flavor/Minimum.pm#L148

ここでconfigに書き込みしてしまっている。

MyApp MyApp::PC -> MyApp MyApp::Admin -> MyApp があるときに(Flavor::Largeで作るとこうなりますね)

MyApp->config; MyApp::PC->config; MyApp::Admin->config; の順に呼ぶと

https://github.com/tokuhirom/Amon/blob/master/lib/Amon2.pm#L46 ここでconfigメソッドが定義されてしまう関係上 その三つの->configが返すのは同じreferenceになってしまうので 最初に提示した view_conf->{path} が、MyApp::PCとMyApp::Adminで一緒になってしまう。

で、ここはまあ事前に

$view_conf = +{ %{$view_conf} };

としてあげれば解決するわけですが もし、Amonのポリシーとしてconfigは読み込み専用としないのであればsub configの実装はまずいのかなと思いました!

という、テストケースもつけないバグ報告ですみません><

nihen avatar Sep 26 '12 14:09 nihen

twitterで@tokuhirom さんとやりとりしたので許可をとりコピペしました!。 (@tokurhiom さんは protected)


tokuhirom: チバさんからきてる issueをみて、すごくごもっともなんだけど、どうやってなおしたらいいのかさっぱりおもいつかない

nihen: configは読み取り専用なので、もし今書き込める実装になってたとしてもそのときの動きは保証しません!でいい気はします。その方針にしたが、Flavorのコードを修正するだけで。

tokurhiom: 書き込み自体はできていいモノなんですよねえ。

nihen: あらま。そうするとポイントはクラスの違うAmon2インスタンス間でconfigは共有されるべきかどうかってとこですね。

tokurhiom: 共有されてもされなくてもいいけど、コストはかからないようにしたい、というかんじですね

nihen: なるほどー。共有される可能性があるのでそれに依存した使い方はやめましょーという感じですかね。

tokuhirom: そうっすねー


というわけで、Flavorだけ治す方向で。のちほどpull reqします。

nihen avatar Sep 27 '12 07:09 nihen

I'm not catching everything here through the translation, but if this a discussion about whether "config" should be read-only or not, my preference would be that it has been read-only.

That policy has worked where for the Config plugins I've developed for CGI::Application. It gives the developer of a complex project the assurance that the config variable that they see in the code has the same value that they see in the config file.

markstos avatar Sep 27 '12 11:09 markstos