Skip to content
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: add gnoutil, centralizing pkg/file matching #1299

Closed
wants to merge 2 commits into from

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Oct 26, 2023

Acting on my comment from a few weeks ago.

This PR introduces a new package, gnoutil, intended as a utility package for dealing with Gno files. Fixes #1056.

The most useful and crucial part of its exported functions is Match, which provides a versatile system for matching gno files in the filesystem. It supersedes gnovm/cmd/gno/util.gno's functions, like gnoPackagesFromArgs or targetsFromPatterns.

I didn't succeed in my intent of having more lines removed than added, but I think if you remove the affected test files, then it should be the case :)

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@thehowl thehowl added the 📦 🤖 gnovm Issues or PRs gnovm related label Oct 26, 2023
@thehowl thehowl self-assigned this Oct 26, 2023
@thehowl thehowl requested review from jaekwon, moul and a team as code owners October 26, 2023 02:23
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Oct 26, 2023
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 117 lines in your changes are missing coverage. Please review.

Comparison is base (9e8fbd3) 47.78% compared to head (7f68232) 47.98%.
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1299      +/-   ##
==========================================
+ Coverage   47.78%   47.98%   +0.20%     
==========================================
  Files         369      370       +1     
  Lines       62702    62791      +89     
==========================================
+ Hits        29963    30132     +169     
+ Misses      30303    30215      -88     
- Partials     2436     2444       +8     
Files Coverage Δ
gnovm/cmd/gno/doc.go 81.81% <100.00%> (ø)
gnovm/cmd/gno/lint.go 89.47% <100.00%> (+5.26%) ⬆️
gnovm/cmd/gno/run.go 88.42% <100.00%> (+5.08%) ⬆️
gnovm/pkg/doc/dirs.go 85.98% <100.00%> (ø)
gnovm/pkg/gnomod/pkg.go 84.53% <ø> (+34.22%) ⬆️
gnovm/cmd/gno/repl.go 42.97% <0.00%> (ø)
gnovm/cmd/gno/util.go 24.48% <0.00%> (-20.81%) ⬇️
gnovm/pkg/gnomod/gnomod.go 69.65% <0.00%> (ø)
gnovm/cmd/gno/build.go 50.00% <33.33%> (-5.36%) ⬇️
gnovm/cmd/gno/precompile.go 40.41% <0.00%> (ø)
... and 4 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -159,7 +159,7 @@ func InitChainer(baseApp *sdk.BaseApp, acctKpr auth.AccountKeeperI, bankKpr bank

// NOTE: comment out to ignore.
if !skipFailingGenesisTxs {
panic(res.Error)
panic(res.Log)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slipping in by accident as most errors here, if we just get res.Error, tend to be internal error. if we feel like we need to discuss it, happy to do another PR.

"github.com/gnolang/gno/tm2/pkg/commands"
)

type buildCfg struct {
verbose bool
goBinary string
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the option to specify a gno file as:

  • this is inconsistent with the rest of guessRootDir, which just use "go" as an argument.
  • it is already fairly straightforward to change this in a script; just change your PATH.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some changing behaviour for this and the following tests: passing . as an argument to gno build is no longer recursive, the simple solution is to use ....

a side effect is that the paths that we return are going to be "simplified" from the input, which is what happens in this and some other files.

// no go/gno tests. disable ellipsis (must be exactly 1 package or file).
gnoutil.MatchFiles(`*.go`, `!/_(file)?test\.{go,gno\.gen\.go}$/`),
gnoutil.MatchEllipsis(false),
gnoutil.MatchNoImplicit(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is possibly stretching the scope a bit (as here we're working on Go files), but given the amount of lines this removes I thought it would still be a good idea.

}
}

sort.Strings(files)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(fs.ReadDir is alphabetical by default, no need to re-sort this)

subPkg.Dir = firstDir

return &subPkg, nil
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(all of this just feels needlessly complex and duplicated logic)

@thehowl thehowl mentioned this pull request Oct 26, 2023
7 tasks
@thehowl
Copy link
Member Author

thehowl commented Nov 2, 2023

This is good for a first review by anyone, but I realised that this PR is missing @/gno.land/pkg/gnoland's GuessGnoRootDir; additionally, some efforts were duplicated with #1233, and finally, it's probably a good idea to add in the kick-start of gnoutil a check to determine whether an import path is of a standard library or an external library (following this comment).

@thehowl
Copy link
Member Author

thehowl commented Mar 26, 2024

Current state: This PR is useful, but needs a better name, and a refactor.

Ultimately, this is a package that aims to centralise all functionality related to import paths, and resolving an import path to corresponding directories and go files. Seeing as Go has a notion of "importer", we can likely call it that.

Detecting the gno root directory should also finish its centralization, but it should remain where it is in gnoenv.

Seeing as #1702 adds an importer already, which would benefit to be moved into this new importer package, I'll wait to work on this again after merging #1702.

It should also ensure to fix the underlying bug in #1306 -- ie. #1142.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Status: 🔵 Not Needed for Launch
Status: No status
Development

Successfully merging this pull request may close these issues.

Refactor the way we categorize files in ReadMemPkg, ParseMemPkg
1 participant