-
Notifications
You must be signed in to change notification settings - Fork 142
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
Unify all state change events on BlockChain<T> into IRenderer<T> #963
Conversation
Codecov Report
@@ Coverage Diff @@
## main #963 +/- ##
==========================================
+ Coverage 88.29% 88.76% +0.47%
==========================================
Files 284 288 +4
Lines 26161 26766 +605
==========================================
+ Hits 23099 23760 +661
+ Misses 1533 1476 -57
- Partials 1529 1530 +1
|
3fe7884
to
99d9f97
Compare
938739a
to
e03b2c9
Compare
const string endMessage = | ||
"Invoked {MethodName}() for an action {ActionType} at block #{BlockIndex}"; | ||
|
||
if (context.Rehearsal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where context.Rehearsal is true during rendering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we correctly implemented and our intention won't change at all there is indeed no such case. However, this is for the sake of easier debugging, so I thought it's useful.
/// An event which is invoked when the chain is reorged. | ||
/// </summary> | ||
public event EventHandler<ReorgedEventArgs> Reorged; | ||
private event EventHandler<(Block<T> OldTip, Block<T> NewTip)> TipChanged; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request introduces a new interface named IRenderer and replaces rendering methods of IAction and TipChanged/Reorged events on BlockChain with it.
Just curiosity... Is this change make replaces TipChanged
event from BlockChain<T>
to IRenderer<T>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curiosity... Is this change make replaces
TipChanged
event fromBlockChain<T>
toIRenderer<T>
?
Yes!:
Besides rendering
IAction
s,IRenderer<T>
also replacesBlockChain<T>.TipChanged
event (introduced in #517 & #526) withIRenderer<T>.RenderBlock()
method, andBlockChain<T>.Reorged
event (introduced in #945) withIRenderer<T>.RenderReorg()
method. It means now all events regarding state changes onBlockChain<T>
become unified into one interfaceIRenderer<T>
.(FYI I actually left
BlockChain<T>.TipChanged
eventprivate
for an internal use: cancelling undergoing mining when a new block is appended. However, still it is invisible from public surface.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I understand. Move rendering role from TipChanged
event to IRenderer<T>.RenderBlock()
. also TipChanged
have only check changed tip blockchain itself. right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@riemannulus Right!
Because without the render option, the same behaviour can be achieved by using non renderers. See also planetarium#883
which is replaced by IRenderer<T>.BlockRender() method.
which is replaced by IRenderer<T>.RenderReorg() method. See also planetarium#945
@planetarium/libplanet I fixed a small typo; please review this again! |
Closes #959.
This pull request introduces a new interface named
IRenderer<T>
and replaces rendering methods ofIAction
andTipChanged
/Reorged
events onBlockChain<T>
with it.To make
IAction
to satisfy separation of concerns, the interface becomes no more aware of rendering, but only hasExecute()
and knows how to serialize and deserialize itself. Instead, rendering mechanism is extracted into the new interface namedIRenderer<T>
, and it needs to be injected intoBlockChain<T>
. In order to watch state changes viaIRenderer<T>
,BlockChain<T>()
constructor now takesrenderers
,IEnumerable<IRenderer<T>>
:Besides rendering
IAction
s,IRenderer<T>
also replacesBlockChain<T>.TipChanged
event (introduced in #517 & #526) withIRenderer<T>.RenderBlock()
method, andBlockChain<T>.Reorged
event (introduced in #945) withIRenderer<T>.RenderReorg()
method. It means now all events regarding state changes onBlockChain<T>
become unified into one interfaceIRenderer<T>
.(FYI I actually left
BlockChain<T>.TipChanged
eventprivate
for an internal use: cancelling undergoing mining when a new block is appended. However, still it is invisible from public surface.)This patch also removed the
render
option from theBlockChain<T>()
constructor (introduced in #883). Asrenderers: null
is equivalent torender: false
, it's redundant.For the sake of convenient development (writing unit tests in particular), I added two built-in renderers as well:
AnonymousRenderer<T>
mimics Java's anonymous class for implementingIRenderer<T>
. If you need only one-use renderer, it helps you to avoid a tedious inner class:LoggedRenderer<T>
decorates anotherIRenderer<T>
with fine-grained logging. Its constructor takes aIRenderer<T>
and a SerilogILogger
, and logs starting and ending point of every event. If any exception happens during event handling it also logs the exception with traceback.