-
Notifications
You must be signed in to change notification settings - Fork 398
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(gnovm/tests/stdlibs): add fmt
#3847
feat(gnovm/tests/stdlibs): add fmt
#3847
Conversation
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
This comment was marked as outdated.
This comment was marked as outdated.
This exists in Go, and it's a pain to add it later as a global, so making a PR to add this. extracted from #3847. --------- Co-authored-by: ltzmaxwell <[email protected]>
cede3cb
to
715d3ed
Compare
This used to return a weird panic, now it correctly panics with a nil pointer dereference. Extracted from #3847. --------- Co-authored-by: ltzmaxwell <[email protected]>
This exists in Go, and it's a pain to add it later as a global, so making a PR to add this. extracted from #3847. --------- Co-authored-by: ltzmaxwell <[email protected]>
- only show the types in the parameters and results for functions, matching go's reflect.Type.String - match field list to go's extracted from #3847. This blocks #3847 because for simplicity, we use the type String() methods for syntaxes like `%#v` and `%T` which show the type. Making these match Go's avoids us modifying some types, and I think it's a nice addition in making us more consistent with Go.
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.
After comparing with the native Go fmt, it follows the original design, and the mini reflection implemented through nativebinding is cleverly designed. LGTM.
Incidentally, I think that this PR may also solve #3687, through implementation of |
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.
LGTM
That issue is still valid for When we move |
This PR removes
fmt
from being implemented using NativeValue / NativeType, and implements a mostly pure-Gno implementation which uses an initial, rudimentary implementation of Gno reflection forfmt
.This PR is ready for review, but some of its related work has been broken out in separate PR to aid reviewing and create atomic PRs.
any
type #3862String
methods #3875Summary of changes
addition of
gnovm/tests/stdlibs/fmt
The main addition is of this set of files, which comprises the bulk of the added lines of this PR. Most of this is a copy of Go's
fmt
. Changes are mostly inerrors.gno
andformat.gno
- the other files are left mostly intact.At the end of
print.gno
, there is an added section containing a set of native functions which form the "reflection" needed to implement thefmt
package in a manner that is similar to Go's. The implementation is not meant to be a public API and is meant to serve initially only for usage byfmt
; we can use this as sketch to then implement a properreflect
package.Attention in reviewing should mostly go to this
print.gno
and itsprint.go
counterpart, especially for any missed cases or potential bugs.addition of
gnovm/tests/stdlibs/os
Complementary to the above, to avoid some problems that were arising due to the handling of os.Stdin, Stdout and Stderr, I also removed the NativeValue / NativeType in
os
and replaced them with a native binding alternative.To avoid having both an
Output
andErrorOutput
on the machine, this package supports an additional method on theMachine.Output
,StderrWrite
, which is implemented by writers who wish to differentiate between the two.other
gnovm/pkg/test
StderrWrite
in its writersos
andfmt
stdin
,stdout
,stderr
into a singleoutput
parameter in the related functions.gnovm/pkg/gnolang
gnovm/tests/stdlibs
, by only testing stdlibs that exist there and not in the official stdlibs.gnovm/cmd/gno
fmt
package.gnovm/tests/files
fmt
package, allowing for instance to show declared types correctly.gnovm/stdlibs/bufio
os.Stdin
, this example had to be changed.misc/genstd
-skip-init-order
when creating it could cause problems, like the testing stdlibs; fixed a bug when usingTypedValue
.