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.

Sign up for GitHub

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

refactor: Add GAZELLE_VERBOSE env and log parser failures #2420

Merged
merged 5 commits into from
Nov 18, 2024
Merged

refactor: Add GAZELLE_VERBOSE env and log parser failures #2420

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
953ad39
feat(gazelle): Add GAZELLE_VERBOSE env and log parser failure.
dougthor42 Nov 17, 2024
534142c
update changelog
dougthor42 Nov 17, 2024
4d9f1c6
spelling
dougthor42 Nov 17, 2024
5982c29
fix typo
rickeylev Nov 18, 2024
d1838e0
Merge branch 'main' into u/dthor/parser-logging
rickeylev Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ Unreleased changes template.

{#v0-0-0-added}
### Added
* Nothing yet.
* (gazelle): Parser failures will now be logged to the terminal. Additional
details can be logged by setting `GAZELLE_VERBOSE=1`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can add this to docs/environment-variables.md, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL! ok I'll add that soon.

And change it to be prefixed with RULES_PYTHON_ to match the others.


{#v0-0-0-removed}
### Removed
Expand Down
35 changes: 32 additions & 3 deletions gazelle/python/file_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package python
import (
"context"
"fmt"
"log"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -55,7 +56,10 @@ func NewFileParser() *FileParser {
return &FileParser{}
}

func ParseCode(code []byte) (*sitter.Node, error) {
// ParseCode instantiates a new tree-sitter Parser and parses the python code, returning
// the tree-sitter RootNode.
// It print a warning if parsing fails.
rickeylev marked this conversation as resolved.
Show resolved Hide resolved
func ParseCode(code []byte, path string) (*sitter.Node, error) {
parser := sitter.NewParser()
parser.SetLanguage(python.GetLanguage())

Expand All @@ -64,9 +68,27 @@ func ParseCode(code []byte) (*sitter.Node, error) {
return nil, err
}

return tree.RootNode(), nil
root := tree.RootNode()
if root.HasError() {
log.Printf("WARNING: failed to parse %q. The resulting BUILD target may be incorrect.", path)

verbose, envExists := os.LookupEnv("GAZELLE_VERBOSE")
if envExists && verbose == "1" {
Comment on lines +75 to +76
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If people are happy with this idea, I will make a more generalized function that can be reused. Maybe even add verbosity levels.

I didn't see any option in the core gazelle that allows for different verbosity levels, so if there is one please LMK.

for i := 0; i < int(root.ChildCount()); i++ {
child := root.Child(i)
if child.IsError() {
log.Printf("Parse error at %+v:\n%+v", child.StartPoint(), child.Content(code))
log.Printf("The above was parsed as: %v", child.String())
Comment on lines +80 to +81
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add comments demo'ing the logged message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lol this was a TODO for me but now it's merged. I'll add this to the followup PR.

}
}
}
}
Comment on lines +72 to +85
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, it is much more idiomatic to do early returns:

Suggested change
if root.HasError() {
log.Printf("WARNING: failed to parse %q. The resulting BUILD target may be incorrect.", path)
verbose, envExists := os.LookupEnv("GAZELLE_VERBOSE")
if envExists && verbose == "1" {
for i := 0; i < int(root.ChildCount()); i++ {
child := root.Child(i)
if child.IsError() {
log.Printf("Parse error at %+v:\n%+v", child.StartPoint(), child.Content(code))
log.Printf("The above was parsed as: %v", child.String())
}
}
}
}
if !root.HasError() {
return root, nil
}
log.Printf("WARNING: failed to parse %q. The resulting BUILD target may be incorrect.", path)
// TODO: if root.HasError, should we still return err=nil?
verbose, envExists := os.LookupEnv("GAZELLE_VERBOSE")
if !envExists || verbose != "1" {
return root, nil
}
for i := 0; i < int(root.ChildCount()); i++ {
child := root.Child(i)
if child.IsError() {
log.Printf("Parse error at %+v:\n%+v", child.StartPoint(), child.Content(code))
log.Printf("The above was parsed as: %v", child.String())
}
}
return root, nil

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! I'll update to that in a followup PR.


return root, nil
}

// parseMain returns true if the python file has an `if __name__ == "__main__":` block,
// which is a common idiom for python scripts/binaries.
func (p *FileParser) parseMain(ctx context.Context, node *sitter.Node) bool {
for i := 0; i < int(node.ChildCount()); i++ {
if err := ctx.Err(); err != nil {
Expand Down Expand Up @@ -94,6 +116,8 @@ func (p *FileParser) parseMain(ctx context.Context, node *sitter.Node) bool {
return false
}

// parseImportStatement parses a node for an import statement, returning a `module` and a boolean
// representing if the parse was OK or not.
func parseImportStatement(node *sitter.Node, code []byte) (module, bool) {
switch node.Type() {
case sitterNodeTypeDottedName:
Expand All @@ -112,6 +136,9 @@ func parseImportStatement(node *sitter.Node, code []byte) (module, bool) {
return module{}, false
}

// parseImportStatements parses a node for import statements, returning true if the node is
// an import statement. It updates FileParser.output.Modules with the `module` that the
// import represents.
func (p *FileParser) parseImportStatements(node *sitter.Node) bool {
if node.Type() == sitterNodeTypeImportStatement {
for j := 1; j < int(node.ChildCount()); j++ {
Expand Down Expand Up @@ -146,6 +173,8 @@ func (p *FileParser) parseImportStatements(node *sitter.Node) bool {
return true
}

// parseComments parses a node for comments, returning true if the node is a comment.
// It updates FileParser.output.Comments with the parsed comment.
func (p *FileParser) parseComments(node *sitter.Node) bool {
if node.Type() == sitterNodeTypeComment {
p.output.Comments = append(p.output.Comments, comment(node.Content(p.code)))
Expand Down Expand Up @@ -180,7 +209,7 @@ func (p *FileParser) parse(ctx context.Context, node *sitter.Node) {
}

func (p *FileParser) Parse(ctx context.Context) (*ParserOutput, error) {
rootNode, err := ParseCode(p.code)
rootNode, err := ParseCode(p.code, p.relFilepath)
if err != nil {
return nil, err
}
Expand Down