-
Notifications
You must be signed in to change notification settings - Fork 389
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(gnodev): lazy loading & staging support #3237
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: gfanton <[email protected]>
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: gfanton <[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
|
Signed-off-by: gfanton <[email protected]>
5b95a3e
to
135f854
Compare
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
logger.Info("guessing directory path", "path", path, "dir", dir) | ||
paths = append(paths, path) // append local path | ||
} else { | ||
logger.Warn("invalid local path", "dir", dir) |
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 think this should be no package found at {pwd}
, maybe even no Gno pacakge found at {pwd}
.
On top of that, maybe another affirmation sentence that will tell the user that it's ok if there is no package, because lazy load kicks in for links that are requested from gnoweb/queries. Something like:
Loader ┃ W no Gno pacakge found at {pwd}
Accounts ┃ I default address imported name=test1 addr=g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5
Node ┃ I packages paths=[] // rm this line in this case
Node ┃ W no path to load, lazy resolving from {GNOROOT}
Or, another approach would to disable the loader output in this case; since querying the node and the given updates should be enough, maybe?
In cases where a package was found or a path was specified, keep it
resolver := packages.NewLocalResolver(path, dir) | ||
|
||
if resolver.IsValid() { | ||
logger.Info("guessing directory path", "path", path, "dir", dir) |
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.
logger.Info("guessing directory path", "path", path, "dir", dir) | |
logger.Info("getting package path", "pkgpath", path, "dir", dir) |
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.
Because in this case it gets it from a gno.mod
file, no?
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.
not necessarily; if no gno.mod
is detected, it will still load the package using the directory as the package name.
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 tested out most of the features manually, lgtm. Really looking forward to this being merged.
I left a few suggestions on the user messaging.
After this we can also set up a small CI job to create a preview of new example realms, or changes to gnoweb
code that would use gnodev
as a base.
Signed-off-by: gfanton <[email protected]>
@leohhhn i've updated gnodev starting logic based on your feedback |
Signed-off-by: gfanton <[email protected]>
featuring:
gnodev
now starts from the current directory by default. The staging subcommand will reproduce previous behaviors by loading and monitoring the entire example folder.Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description