-
Notifications
You must be signed in to change notification settings - Fork 373
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
feat(gno): use verbose
flag to print emitted events
#2071
feat(gno): use verbose
flag to print emitted events
#2071
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2071 +/- ##
==========================================
+ Coverage 60.95% 60.99% +0.03%
==========================================
Files 564 564
Lines 75273 75290 +17
==========================================
+ Hits 45885 45925 +40
+ Misses 26018 25995 -23
Partials 3370 3370
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Events printing should be as common as stdout. It should be the default on gnokey maketx
and similar commands.
Regarding gnotest, I'm considering whether your approach is the best or if we should print nothing by default and only print everything (stdout and events) when using the -v
option.
Blocking. We should wait for more feedback before making a decision.
Hey @moul, here is the issue with the discussion about this - TL;DR I am in favor of adding |
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've left a few comments that need addressing before we can move forward, otherwise it looks good 💯
gnovm/cmd/gno/test.go
Outdated
res := gno.Call("runtest", testFuncStr) | ||
|
||
eval := m.Eval(res) // NOTE: verbose prints get here | ||
if printEvents { |
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 agree with @moul, I think we should print these events if the verbose
flag is enabled, and not hide this functionality behind an additional flag we need to now maintain
The reasoning is that events are now an integral part of the Gno development lifecycle, and not a "feature" we can disable
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.
The reasoning is that events are now an integral part of the Gno development lifecycle
I totally understand, however for easier(or at least more readable) debugging flags needs to be separated.
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.
875394f
to
2cf4dec
Compare
If stdout is printed, events should also be printed if they exist. Currently, events should be considered as returned values of a contract, representing an outcome or consequence as the returned value (stdout). Let's wait for a clear example to determine if events are too spammy. Initially, we should consider reducing the number of events. If they overwhelm users, it makes the contract's usage more implicit, which we aim to avoid. We should always print events when stdout is printed and maintain our usual rule: strive for straightforward, explicit, and simple communication without hiding things as soon as they become complex; let's just remove the complex. |
-print-events
flag to gno test
to print emitted eventsverbose
flag to print emitted events
@zivkovicmilos And CI fails on codecov against |
Closes half of #2007
Add
print-events
flag to print emitted events ingno -test
.Sample
Ouput
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description