-
Notifications
You must be signed in to change notification settings - Fork 143
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
DelayedRenderer<T> & DelayedActionRenderer<T> for stable app-side state transfer #980
Conversation
db3040b
to
2aa3cb9
Compare
Codecov Report
@@ Coverage Diff @@
## main #980 +/- ##
==========================================
+ Coverage 88.88% 89.04% +0.16%
==========================================
Files 320 324 +4
Lines 28132 29060 +928
==========================================
+ Hits 25004 25876 +872
- Misses 1598 1629 +31
- Partials 1530 1555 +25
|
4319d95
to
7a084d8
Compare
…ock<T>> as well [changelog skip]
7a084d8
to
e09460d
Compare
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.
I'll continue to review it again today.
e09460d
to
a5fb352
Compare
IActionRenderer<DumbAction> renderer = new AnonymousActionRenderer<DumbAction> | ||
{ | ||
ActionRenderer = (_, __, nextStates) => | ||
throw new SomeException("thrown by renderer"), | ||
}; | ||
renderer = new LoggedActionRenderer<DumbAction>(renderer, Log.Logger); |
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.
Isn't it overwritten?
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.
That's the way to use LoggedActionRenderer<T>
as it uses decorator pattern. See also the example code in its docs.
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.
I will review furthermore.
a5fb352
to
0ba1b20
Compare
0ba1b20
to
ad32f96
Compare
@planetarium/libplanet I make multiple renderers guaranteed to get their own distinct action contexts (i.e., one renderer consuming its random state does not affect other one's random state): 4e62c1b. Please review this again! |
{ | ||
throw new ArgumentException( | ||
"Some previous blocks of two blocks are orphan.", | ||
nameof(a) |
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.
ArgumentException
seems designed for only one argument. 🤔
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.
Indeed I had thought the same question, and anyway I have no idea…
to use delayed renderers. See also: planetarium/libplanet#980
to use delayed renderers. See also: planetarium/libplanet#980
This patch adds
DelayedRenderer<T>
&DelayedActionRenderer<T>
that decorate anotherIRenderer<T>
&IActionRenderer<T>
and delay rendering events until the certain number of descendant blocks confirm a block. See also the unit tests and XML comments.