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(website): add godoc-style documentation to gno packages #526

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

yassinouider
Copy link
Contributor

Add new documentation to the package pages on gno.land, allowing developers to view information like pkg.go.dev.

Currently, the package pages on gno.land only display a list of files. I'm submitting this pull request to propose a new feature that will extract and generate documentation for Gno packages, similar to the way Godoc works for Go programs. For the moment it's a basic version, I just saw issue #522 which may bring a more complete version.

Gno.land.-.pkg.doc.mp4

@yassinouider yassinouider requested a review from a team as a code owner February 18, 2023 17:31
@moul
Copy link
Member

moul commented Feb 18, 2023

Hey @yassinouider, thank you for this contribution.

Yep, it’s related to #522 and going in the good direction.
It can be related with #498 too.

—-

@thehowl, could you review this and consider integrating it with your work on #522 and suggest the next steps, please?

@moul moul changed the title website: add godoc-style documentation to package gno feat(website): add godoc-style documentation to package gno Feb 18, 2023
Fixed the issue causing the golangci linter to fail by applying recommended changes to the code. Additionally, removed the feature for rendering package comments in the documentation template until a better solution is found.
@thehowl
Copy link
Member

thehowl commented Feb 19, 2023

thank you for your contribution :) 🎉

I'll take a look first thing on Monday

@moul moul mentioned this pull request Feb 20, 2023
16 tasks
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Numerous places for improvement, but the code is in a good state and I really like the feature 👍

Could you also add unit tests for pkgs/doc?

gnoland/website/views/package_dir.html Outdated Show resolved Hide resolved
gnoland/website/static/css/app.css Outdated Show resolved Hide resolved
gnoland/website/views/package_dir.html Outdated Show resolved Hide resolved
gnoland/website/views/package_dir.html Outdated Show resolved Hide resolved
gnoland/website/views/package_dir.html Outdated Show resolved Hide resolved
gnoland/website/pkgs/doc/types.go Outdated Show resolved Hide resolved
gnoland/website/pkgs/doc/types.go Outdated Show resolved Hide resolved
gnoland/website/pkgs/doc/types.go Outdated Show resolved Hide resolved
gnoland/website/pkgs/doc/doc.go Show resolved Hide resolved
gnoland/website/main.go Outdated Show resolved Hide resolved
@yassinouider
Copy link
Contributor Author

Numerous places for improvement, but the code is in a good state and I really like the feature 👍

Could you also add unit tests for pkgs/doc?

Yes, it is a first effort but there is still a lot to do.
I'm looking at adding the tests and making the changes.

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.

I love the feature idea, it looks very slick 💯

@thehowl did a pretty good job of reviewing this PR already. I've also left some minor comments that should be addressed.

Please see @thehowl's open discussions and issues, and after resolving them we can go from there

pkgs/sdk/vm/handler.go Outdated Show resolved Hide resolved
pkgs/sdk/vm/keeper.go Outdated Show resolved Hide resolved
pkgs/sdk/vm/keeper.go Outdated Show resolved Hide resolved
gnoland/website/static/css/app.css Outdated Show resolved Hide resolved
gnoland/website/pkgs/doc/types.go Outdated Show resolved Hide resolved
gnoland/website/pkgs/doc/types.go Outdated Show resolved Hide resolved
gnoland/website/pkgs/doc/types.go Outdated Show resolved Hide resolved
@yassinouider yassinouider requested review from thehowl and zivkovicmilos and removed request for thehowl and zivkovicmilos February 26, 2023 09:15
@yassinouider
Copy link
Contributor Author

@moul @zivkovicmilos @thehowl when I think I have solved some reviews, should I close the conversation or you after checking ?

@moul
Copy link
Member

moul commented Feb 26, 2023

We'll do.

gnoland/website/pkgs/doc/ast_to_string_test.go Outdated Show resolved Hide resolved
gnoland/website/pkgs/doc/doc.go Outdated Show resolved Hide resolved
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Don't know if GitHub has already sent the comments, but 🤷

Resolved the conversations which you addressed, there still a few outstanding points as far as I can see

gnoland/website/pkgs/doc/doc_test.go Outdated Show resolved Hide resolved
gnoland/website/pkgs/doc/doc_test.go Outdated Show resolved Hide resolved
gnoland/website/views/funcs.html Outdated Show resolved Hide resolved
@thehowl
Copy link
Member

thehowl commented Mar 8, 2023

I didn't necessarily mean changing everything to Markdown, but this may be a good idea considering the spirit of the project and the idea of the defunct logos browser. In any case, I'll request a last round of reviews from Manfred and Miloš.

As one last note, I also think it's probably better to add a few more test cases for code that is not covered yet, namely:

  • typeString on variadic arguments (ie. add a doc test for a function with variadic arguments)
  • typeString on named return variables
  • A test ensuring New ignores _file.gno and _filetest.gno
  • Adding on doc_test.go a test ensuring that blocks of mixed exported/unexported types are documented correctly
  • Adding on doc_test.go a test for assignments on multiple variables (ie., x, err = Func(), x, y = 1, 2)

@@ -193,6 +197,22 @@ func (vh vmHandler) queryFile(ctx sdk.Context, req abci.RequestQuery) (res abci.
return
}

func (vh vmHandler) queryFiles(ctx sdk.Context, req abci.RequestQuery) (res abci.ResponseQuery) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func (vh vmHandler) queryFiles(ctx sdk.Context, req abci.RequestQuery) (res abci.ResponseQuery) {
func (vh vmHandler) queryPkgFiles(ctx sdk.Context, req abci.RequestQuery) (res abci.ResponseQuery) {

Copy link
Member

Choose a reason for hiding this comment

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

After more reflection, I think we should not have a queryPkgFiles handler that gets the bodies, but instead implement the getPackage helper returning a list of files with attributes and not only the body: https://github.com/gnolang/gno/pull/526/files#diff-86850325e38fa997f532d762282030b3e585caa785324e2187eceba36e8169d5R121

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid going in the wrong direction, you want me to return the std.MemPackage directly?

//keeper.go
func (vm *VMKeeper) QueryPackage(ctx sdk.Context, path string) (res std.MemPackage, err error) {
	dirpath, _ := std.SplitFilepath(path)
	store := vm.getGnoStore(ctx)
	if memPkg := store.GetMemPackage(dirpath); memPkg != nil {
		return memPkg, nil
	}

	return std.MemPackage{}, nil
}

//handler.go
func (vh vmHandler) queryPackage(ctx sdk.Context, req abci.RequestQuery) (res abci.ResponseQuery) {
	dirpath := string(req.Data)
	result, err := vh.vm.QueryPackage(ctx, dirpath)
	if err != nil {
		res = sdk.ABCIResponseQueryFromError(err)
		return
	}
	b, err := json.Marshal(&result)
	if err != nil {
		res = sdk.ABCIResponseQueryFromError(err)
		return
	}
	res.Data = b
	return 
}

Copy link
Member

Choose a reason for hiding this comment

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

It may be beneficial to keep the API as concise as possible, rather than having numerous options that essentially perform the same task.

@@ -462,3 +462,13 @@ func (vm *VMKeeper) QueryFile(ctx sdk.Context, filepath string) (res string, err
return res, nil
}
}

func (vm *VMKeeper) QueryFiles(ctx sdk.Context, path string) (res std.MemFileBodies, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func (vm *VMKeeper) QueryFiles(ctx sdk.Context, path string) (res std.MemFileBodies, err error) {
func (vm *VMKeeper) QueryPkgFiles(ctx sdk.Context, path string) (res std.MemFileBodies, err error) {

@@ -30,6 +32,14 @@ func (mempkg *MemPackage) GetFile(name string) *MemFile {
return nil
}

func (mempkg *MemPackage) GetFileBodies() MemFileBodies {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func (mempkg *MemPackage) GetFileBodies() MemFileBodies {
func (mempkg *MemPackage) GetFileBodies() map[string]string {

If you anticipate needing more helpers in the future, you can disregard this suggestion. However, if not, I believe this approach will be more discoverable and easier to upgrade later.

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

[Blocking comment] to ensure we review the new API endpoints before merging.

@moul
Copy link
Member

moul commented Mar 8, 2023

Please, look at this comment first, it's IMO the starting point -> #526 (comment)

@thehowl
Copy link
Member

thehowl commented Jun 19, 2023

@yassinouider are you still interested in working on this?

@moul moul added this to the 💡Someday/Maybe milestone Sep 6, 2023
@zivkovicmilos
Copy link
Member

@thehowl
Do you want to take up this PR, or do we want to close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔵 Not Needed for Launch
Status: No status
Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants