Closed andig closed 1 year ago
I like the idea of this, I would use the name Logger
vs WithLogger
personally, will review once you have finished up.
I've updated. name is still WithConsole
or it would clash with the Console
interface. Looking at the remaining error I've realised that while this PR gives a nicer interface, it really doesn't seem to be necessary as you can already use vm.Set("console", ...)
to achieve the same result.
So not sure- is this still beneficial?
@stevenh what do you think?
Love this idea! 👍🏼
Love this idea! 👍🏼
------------------ 原始邮件 ------------------ 发件人: "Dan @.>; 发送时间: 2022年11月30日(星期三) 上午9:47 收件人: @.>; 抄送: @.***>; 主题: Re: [robertkrimen/otto] Add a console option (PR #445)
Love this idea! 👍🏼
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: @.***>
Yikes. Why does the latest commit suddenly have conflicts?
Lots of cleanup in the last week or so. Should be pretty simple to apply your work on top of the latest master.
@andig Any interest in finishing up this PR? Looks like a great change to me and I would probably otherwise have to implement this myself later.
If not, permission to take your work and continue? 😉
Please grab it, thank you!
@andig Yesterday I squashed all changes and then tried rebasing, was kind of difficult. In any case I will experiment but maybe start fresh. Thanks for your draf anyhow! Continuing in https://github.com/FabianTe/otto/issues/1 and an eventually linked PR.
This PR adds an options pattern to the
otto.New()
function and provides a per-VMWithLogger()
option utilising a logger interface. At this point it is more a proof of concept. We'd still need to determine if this should be aLogger
or aConsole
interface.Note: I realise this overlaps with https://github.com/robertkrimen/otto/pull/341, I'd be happy to make this a
ConsoleWriter
. I'd maintain that it should be per VM.