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: implement gno mod init command #955

Merged
merged 25 commits into from
Aug 9, 2023
Merged

Conversation

harry-hov
Copy link
Contributor

@harry-hov harry-hov commented Jul 6, 2023

Implement gno mod init command.

Usage:

$ gno mod init <module-path>

If no <module-path> is given, tries to determine package name from existing *.gno files.

Checklists...

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.

Maintainers Checklist

  • Checked that the author followed the guidelines in CONTRIBUTING.md
  • Checked the conventional-commit (especially PR title and verb, presence of BREAKING CHANGE: in the body)
  • Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jul 6, 2023
@harry-hov harry-hov marked this pull request as ready for review July 7, 2023 23:37
@harry-hov harry-hov requested a review from a team as a code owner July 7, 2023 23:37
gnovm/pkg/gnomod/file.go Outdated Show resolved Hide resolved
@harry-hov harry-hov requested review from moul and a team July 12, 2023 20:40
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks good 💯

Really welcoming this feature 🚀

ShortUsage: "init [module-path]",
ShortHelp: "Initialize gno.mod file in current directory",
},
nil,
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: you can use commands.NewEmptyConfig(), as with other commands that don't have a config file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: 791996d

gnovm/pkg/gnomod/file.go Show resolved Hide resolved
modPath = string(pkgName)
}

modFile := &File{
Copy link
Member

Choose a reason for hiding this comment

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

Why not create a helper for this initialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I agree there is a helper needed for File initialization.
I plan to refactor code and define methods for File. like for this specific case modFile.AddModuleStmt().
Will do it in the another PR.

Comment on lines +209 to +212
if pkgName == "" {
return errors.New("cannot determine package name")
}
modPath = string(pkgName)
Copy link
Member

Choose a reason for hiding this comment

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

What is this situation when pkgName == """?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

Should be fixed by: fa7fc5a

// Verify gno.mod file
bz, err := os.ReadFile(filepath.Join(dirPath, "gno.mod"))
assert.NoError(t, err)
assert.Equal(t, string(bz), tc.out)
Copy link
Member

Choose a reason for hiding this comment

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

The second param for assert.Equal is the expected, not the actual (so the params should be switched)

Copy link
Contributor Author

@harry-hov harry-hov Jul 27, 2023

Choose a reason for hiding this comment

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

Opps.

Fixed here: 58f6ffa, 7807a32

"github.com/stretchr/testify/require"
)

func TestCreateGnoModFile(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you paralellize this test with t.Parallel()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done efc2e5b

@harry-hov harry-hov mentioned this pull request Aug 9, 2023
7 tasks

var pkgName gnolang.Name
for _, file := range files {
if file.IsDir() || !strings.HasSuffix(file.Name(), ".gno") || strings.HasSuffix(file.Name(), "_filetest.gno") {
Copy link
Member

Choose a reason for hiding this comment

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

We should create helpers for this in another PR. Also discussed with @tbruyelle in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Will open PR for that soon.

@moul moul merged commit a612e57 into gnolang:master Aug 9, 2023
Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023
Implement `gno mod init` command.

Usage: 
```sh
$ gno mod init <module-path>
```
If no `<module-path>` is given, tries to determine package name from
existing `*.gno` files.

<details><summary>Checklists...</summary>

## Contributors Checklist

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

## Maintainers Checklist

- [x] Checked that the author followed the guidelines in
`CONTRIBUTING.md`
- [x] Checked the conventional-commit (especially PR title and verb,
presence of `BREAKING CHANGE:` in the body)
- [x] Ensured that this PR is not a significant change or confirmed that
the review/consideration process was appropriate for the change
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: 🌟 Wanted for Launch
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants