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 vm/qdoc with function comments #3459

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

Conversation

jefft0
Copy link
Contributor

@jefft0 jefft0 commented Jan 8, 2025

Addresses #522 (comment)
by adding a query for vm/qdoc that uses a similar code path to gno doc which includes comments.

For example, gnokey query vm/qdoc -data "gno.land/r/gnoland/valopers/v2" -remote tcp://127.0.0.1:26657 returns the following JSON doc for valopers.

{
  "package_path": "gno.land/r/gnoland/valopers/v2",
  "package_line": "package valopers // import \"valopers\"",
  "package_doc": "Package valopers is designed around the permissionless lifecycle of valoper profiles. It also includes parts designed for govdao to propose valset changes based on registered valopers.\n",
  "values": [
    {
      "signature": "const (\n\terrValoperExists        = \"valoper already exists\"\n\terrValoperMissing       = \"valoper does not exist\"\n\terrInvalidAddressUpdate = \"valoper updated address exists\"\n\terrValoperNotCaller     = \"valoper is not the caller\"\n)",
      "doc": ""
    },
    {
      "signature": "var valopers *avl.Tree // Address -\u003e Valoper\n",
      "doc": "valopers keeps track of all the active validator operators\n"
    }
  ],
  "funcs": [
    {
      "type": "",
      "name": "GovDAOProposal",
      "signature": "func GovDAOProposal(address std.Address)",
      "doc": "GovDAOProposal creates a proposal to the GovDAO for adding the given valoper to the validator set. This function is meant to serve as a helper for generating the govdao proposal\n"
    },
    {
      "type": "",
      "name": "Register",
      "signature": "func Register(v Valoper)",
      "doc": "Register registers a new valoper\n"
    },
    {
      "type": "",
      "name": "Render",
      "signature": "func Render(_ string) string",
      "doc": "Render renders the current valoper set\n"
    },
    {
      "type": "",
      "name": "Update",
      "signature": "func Update(address std.Address, v Valoper)",
      "doc": "Update updates an existing valoper\n"
    },
    {
      "type": "",
      "name": "init",
      "signature": "func init()",
      "doc": ""
    },
    {
      "type": "",
      "name": "isValoper",
      "signature": "func isValoper(address std.Address) bool",
      "doc": "isValoper checks if the valoper exists\n"
    },
    {
      "type": "Valoper",
      "name": "Render",
      "signature": "func (v Valoper) Render() string",
      "doc": "Render renders a single valoper with their information\n"
    }
  ],
  "types": [
    {
      "name": "Valoper",
      "signature": "type Valoper struct {\n\tName        string // the display name of the valoper\n\tMoniker     string // the moniker of the valoper\n\tDescription string // the description of the valoper\n\n\tAddress      std.Address // The bech32 gno address of the validator\n\tPubKey       string      // the bech32 public key of the validator\n\tP2PAddresses []string    // the publicly reachable P2P addresses of the validator\n\tActive       bool        // flag indicating if the valoper is active\n}",
      "doc": "Valoper represents a validator operator profile\n"
    }
  ]
}

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jan 8, 2025
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Jan 8, 2025

🛠 PR Checks Summary

🔴 Changes to 'docs' folder must be reviewed/authored by at least one devrel and one tech-staff

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
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)
🔴 Changes to 'docs' folder must be reviewed/authored by at least one devrel and one tech-staff
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: jefft0/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Changes to 'docs' folder must be reviewed/authored by at least one devrel and one tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 A changed file matches this pattern: ^docs/ (filename: docs/gno-tooling/cli/gnokey/querying-a-network.md)

Then

🔴 Requirement not satisfied
└── 🔴 And
    ├── 🔴 Or
    │   ├── 🔴 Pull request author is a member of the team: tech-staff
    │   └── 🔴 At least 1 user(s) of the team tech-staff reviewed pull request(with state "APPROVED")
    └── 🔴 Or
        ├── 🔴 Pull request author is a member of the team: devrels
        └── 🔴 At least 1 user(s) of the team devrels reviewed pull request(with state "APPROVED")

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🔴 At least 1 user(s) of the organization reviewed the pull request (with state "APPROVED")
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

@jefft0 jefft0 changed the title feat: In doc, add newPkgDataFromMemPkg feat: Add vm/qdoc with func comments Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 32.30769% with 88 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gnovm/pkg/doc/json_doc.go 0.00% 81 Missing ⚠️
gnovm/pkg/doc/pkg.go 76.47% 4 Missing ⚠️
gno.land/pkg/sdk/vm/keeper.go 78.57% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@jefft0 jefft0 force-pushed the feat/wm-qdoc-with-comments branch 3 times, most recently from c2538d6 to b295454 Compare January 13, 2025 16:37
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jan 13, 2025
@Gno2D2 Gno2D2 requested review from a team January 13, 2025 16:38
@jefft0 jefft0 changed the title feat: Add vm/qdoc with func comments feat: Add vm/qdoc with function comments Jan 14, 2025
@jefft0 jefft0 force-pushed the feat/wm-qdoc-with-comments branch 2 times, most recently from fc953be to a2a8856 Compare January 16, 2025 11:12
@Gno2D2 Gno2D2 requested a review from a team January 16, 2025 12:07
@thehowl
Copy link
Member

thehowl commented Jan 16, 2025

type JSONDocumentation struct {
	PkgPath string `json:"pkg_path"`
	PackageLine string `json:...` // package io // import "io"
	PackageDoc string // markdown of top-level package documentation
	// https://pkg.go.dev/go/doc#Package.Markdown to render markdown
	
	// These match each of the sections in a pkg.go.dev package documentationj
	Values []JSONValue // constants and variables declared
	Funcs []JSONFunc // ...
	Types []JSONType // ...
}

type JSONFunc struct {
	Name string
	Signature string
	Doc string // markdown
}

cc @alexiscolin @Kouteki to draft an idea for what we want for the frontend.

Ideally, we'd base it off a go package documentation (like io), but also add boxes to retain the functionality of the help page on realm documentation (so we can compose easily gnokey commands from the gnoweb frontend).

@jefft0 jefft0 force-pushed the feat/wm-qdoc-with-comments branch 2 times, most recently from 8da68ae to 52f5689 Compare January 20, 2025 08:39
@jefft0
Copy link
Contributor Author

jefft0 commented Jan 20, 2025

Hi @thehowl . Currently, gno doc creates a Documentable which has the AST, then calls WriteDocumentation which writes directly to an io.Writer. It seems that we should have an intermediate method of Documentable like GetJSONDocumentation which creates a JSONDocumentation. Then WriteDocumentation would internally call this, then write to the io.Writer . Does that make sense? (Of course, the motivation is that vm/qdoc can just call GetJSONDocumentation .)

@thehowl
Copy link
Member

thehowl commented Jan 20, 2025

@jefft0 I'd say:

  1. Let's remove the Documentable interface, and instead export the documentable struct.
  2. Let's make a WriteJSONDocumentation method
  3. I think these can likely both work off of the pkgData.docPackage method, or some other common method that collects information about the package to then be presented into the desired output.

sounds good?

@jefft0
Copy link
Contributor Author

jefft0 commented Jan 23, 2025

Hi @thehowl . I'm making progress with your suggestion. I got blocked for a while because I didn't understand that pkgData.docPackage modifies the pkgData. If you call it twice, then the result of the second call doesn't have comments. (You can imagine how undesirable that was since I'm trying to get comments!) Ultimately this is because it calls doc.New which "takes ownership of the AST pkg and may edit or overwrite it". I think I can work around this. But do you think we should update the comments for pkgData.docPackage to warn callers, or try to make it not have this behavior?

@jefft0 jefft0 force-pushed the feat/wm-qdoc-with-comments branch from 52f5689 to 566aa77 Compare January 24, 2025 15:10
@jefft0 jefft0 force-pushed the feat/wm-qdoc-with-comments branch from 566aa77 to 752c34b Compare January 24, 2025 15:21
@jefft0 jefft0 force-pushed the feat/wm-qdoc-with-comments branch from 752c34b to 45a8c82 Compare January 24, 2025 15:39
@jefft0
Copy link
Contributor Author

jefft0 commented Jan 24, 2025

@thehowl, please see the updated PR description with example output. As you suggested above, I changed Documentatable from an interface to a struct and added WriteJSONDocumentation .

Right now, it adds all the functions, etc. for the package. If only one symbol is selected, I'm not sure how one would use vm/qdoc to return a JSONDocumentation which only populates the one symbol. If the code so far looks good, we can discuss it.

@jefft0 jefft0 marked this pull request as ready for review January 24, 2025 15:42
@jefft0 jefft0 marked this pull request as draft January 24, 2025 16:58
@jefft0 jefft0 force-pushed the feat/wm-qdoc-with-comments branch from 957653c to 9ced645 Compare January 24, 2025 17:04
Signed-off-by: Jeff Thompson <[email protected]>
@jefft0 jefft0 force-pushed the feat/wm-qdoc-with-comments branch from 9ced645 to cc96681 Compare January 24, 2025 17:05
@jefft0 jefft0 marked this pull request as ready for review January 24, 2025 17:07
@Gno2D2 Gno2D2 requested a review from a team January 24, 2025 18:44
@jefft0
Copy link
Contributor Author

jefft0 commented Jan 31, 2025

Can you please share a short update on how you see this integrated with gnoweb?

Ultimately, we want help_function.html to display the function comment next to the function name. Function information is passed in the HelpData type. Its Functions member is an array of vm.FunctionSignature which doesn't have comments. So we need to define a new struct similar to vm.FunctionSignature but with comments, and use it in HelpData.

Currently, HelpData is filled in by GetHelpView which calls vm/qfuncs using the helper Client.Functions. We need to add a similar helper Client.Doc which calls vm/qdoc to get the comments for all the functions in the package. Then GetHelpView needs to add the comments into the new struct used for function info in HelpData.
In this PR, the API for vm/qdoc returns a JSONDocumentation with an array of JSONFunc. So, for each function in the result from vm/qfuncs, GetHelpView needs to search the result from vm/qdoc for the matching function to get its comment. The only alternative for the vm/qdoc API that I can think of would be for JSONDocumentation to have a map of function name to JSONFunc. (Or the new helper helper Client.Doc should create this map.) This would make the lookup easier.

expectedErrorMatch string
}{
// valid queries
{input: []byte(`gno.land/r/hello`), expectedResult: `{"package_path":"gno.land/r/hello","package_line":"package hello // import \"hello\"","package_doc":"hello is a package for testing\n","values":[{"signature":"const ConstString = \"const string\"","doc":""},{"signature":"var PubString = \"public string\"","doc":""},{"signature":"var counter int = 42","doc":""},{"signature":"var myStructInst = myStruct{a: 1000}","doc":""},{"signature":"var pvString = \"private string\"","doc":""},{"signature":"var sl = []int{1, 2, 3, 4, 5}","doc":"sl is an int array\n"}],"funcs":[{"type":"","name":"Echo","signature":"func Echo(msg string) string","doc":""},{"type":"","name":"GetCounter","signature":"func GetCounter() int","doc":""},{"type":"","name":"Inc","signature":"func Inc() int","doc":""},{"type":"","name":"Panic","signature":"func Panic()","doc":"Panic is a func for testing\n"},{"type":"","name":"fn","signature":"func fn() func(string) string","doc":""},{"type":"","name":"pvEcho","signature":"func pvEcho(msg string) string","doc":""},{"type":"myStruct","name":"Foo","signature":"func (ms myStruct) Foo() string","doc":"Foo is a method for testing\n"}],"types":[{"name":"myStruct","signature":"type myStruct struct{ a int }","doc":"myStruct is a struct for testing\n"}]}`},
Copy link
Member

Choose a reason for hiding this comment

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

Can we have maybe the result as a JSONDocumentation to compare to? Easier to check and verify than this huge JSON blob.

jsonDoc.Funcs = append(jsonDoc.Funcs, &JSONFunc{
Name: fun.Name,
Signature: buf.String(),
Doc: string(pkg.Markdown(fun.Doc)),
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
Doc: string(pkg.Markdown(fun.Doc)),
Doc: string(pkg.Markdown(fun.Doc)),
Params: []JSONField{...},
Results: []JSONField{...},

JSONField is a simple struct{ Name string; Type string }, breaking down the ast.Field type

Comment on lines +26 to +29
type JSONValue struct {
Signature string `json:"signature"`
Doc string `json:"doc"` // markdown
}
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
type JSONValue struct {
Signature string `json:"signature"`
Doc string `json:"doc"` // markdown
}
type JSONValueDecl struct {
Signature string `json:"signature"`
Const bool `json:"const"`
Values []JSONValue `json:"values"`
Doc string `json:"doc"` // markdown
}
type JSONValue struct {
Name string `json:"name"`
Doc string `json:"doc"`
Type string `json:"type"` // often empty
}

@Gno2D2 Gno2D2 requested a review from a team February 5, 2025 11:33
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: In Review
Development

Successfully merging this pull request may close these issues.

5 participants