-
Notifications
You must be signed in to change notification settings - Fork 385
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): categorize imports #3323
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Norman Meier <[email protected]>
🛠 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! |
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
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.
makes sense overall, some comments
gnovm/pkg/packages/filekind.go
Outdated
type FileKind uint | ||
|
||
const ( | ||
FileKindUnknown = FileKind(iota) |
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.
FileKindUnknown = FileKind(iota) | |
FileKindUnknown FileKind = iota |
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.
@@ -13,34 +13,40 @@ import ( | |||
) | |||
|
|||
// Imports returns the list of gno imports from a [gnovm.MemPackage]. | |||
// fset is optional. |
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.
Would still keep it as a comment
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.
my bad, conflict resolution fail: 41ddaa6
|
||
import ( | ||
"os" | ||
"path/filepath" | ||
"testing" | ||
|
||
"github.com/gnolang/gno/gnovm/pkg/gnolang" | ||
. "github.com/gnolang/gno/gnovm/pkg/packages" |
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.
why?
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 saw in the go standard libraries sources that they are using dot import for the current package in external tests, thus thought it was the idiomatic go way. I can use a normal import if you prefer
gnovm/pkg/packages/filekind.go
Outdated
|
||
const ( | ||
FileKindUnknown = FileKind(iota) | ||
FileKindCompiled |
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.
FileKindCompiled | |
FileKindPackageSource |
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.
gnovm/pkg/packages/filekind.go
Outdated
FileKindUnknown = FileKind(iota) | ||
FileKindCompiled | ||
FileKindTest | ||
FileKindXtest |
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.
Go uses XTest: https://pkg.go.dev/go/build
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.
"strings" | ||
) | ||
|
||
type FileKind uint |
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.
Please add godoc
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.
Co-authored-by: Morgan <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
This makes the imports utils split imports by file kinds, allowing to make explicit decisions about what imports to use at the various callsites
FileKind
enum to categorize gno files, with variantsPackageSource
,Test
,XTest
andFiletest
GetFileKind
util to derive theFileKind
from a file name and bodyImportsMap
type that mapsFileKind
s to lists of imports. It has a single methodMerge
to select and merge various imports from multipleFileKind
spackages.Imports
helper to return anImportsMap
instead of a[]string
and adapt callsites by usingImportMap.Merge
to preserve existing behaviorThis is something I need for #3304 and #2932 but to help reviews I made an atomic PR here instead