-
Notifications
You must be signed in to change notification settings - Fork 532
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(gazelle): Add "include_dep" Python comment annotation #1863
Changes from 14 commits
3539d51
ce9ee9d
f2e42ad
fbc02f3
0fe5750
eccf7bd
92adfc1
cdf2a49
975011b
4cbfecd
5ad8471
ae8e200
b5f3bf9
6f0b7eb
4d15f7d
a00b5da
8832835
8da48cb
5a5a563
ac6151e
98c61fb
8c68bdb
cb650d7
43f82a8
ba2ed39
090c178
b1bfd1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,15 +101,15 @@ func newPython3Parser( | |
|
||
// parseSingle parses a single Python file and returns the extracted modules | ||
// from the import statements as well as the parsed comments. | ||
func (p *python3Parser) parseSingle(pyFilename string) (*treeset.Set, map[string]*treeset.Set, error) { | ||
func (p *python3Parser) parseSingle(pyFilename string) (*treeset.Set, map[string]*treeset.Set, *annotations, error) { | ||
pyFilenames := treeset.NewWith(godsutils.StringComparator) | ||
pyFilenames.Add(pyFilename) | ||
return p.parse(pyFilenames) | ||
} | ||
|
||
// parse parses multiple Python files and returns the extracted modules from | ||
// the import statements as well as the parsed comments. | ||
func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[string]*treeset.Set, error) { | ||
func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[string]*treeset.Set, *annotations, error) { | ||
parserMutex.Lock() | ||
defer parserMutex.Unlock() | ||
|
||
|
@@ -122,28 +122,29 @@ func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[strin | |
} | ||
encoder := json.NewEncoder(parserStdin) | ||
if err := encoder.Encode(&req); err != nil { | ||
return nil, nil, fmt.Errorf("failed to parse: %w", err) | ||
return nil, nil, nil, fmt.Errorf("failed to parse: %w", err) | ||
} | ||
|
||
reader := bufio.NewReader(parserStdout) | ||
data, err := reader.ReadBytes(0) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("failed to parse: %w", err) | ||
return nil, nil, nil, fmt.Errorf("failed to parse: %w", err) | ||
} | ||
data = data[:len(data)-1] | ||
var allRes []parserResponse | ||
if err := json.Unmarshal(data, &allRes); err != nil { | ||
return nil, nil, fmt.Errorf("failed to parse: %w", err) | ||
return nil, nil, nil, fmt.Errorf("failed to parse: %w", err) | ||
} | ||
|
||
mainModules := make(map[string]*treeset.Set, len(allRes)) | ||
allAnnotations := new(annotations) | ||
for _, res := range allRes { | ||
if res.HasMain { | ||
mainModules[res.FileName] = treeset.NewWith(moduleComparator) | ||
} | ||
annotations, err := annotationsFromComments(res.Comments) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("failed to parse annotations: %w", err) | ||
return nil, nil, nil, fmt.Errorf("failed to parse annotations: %w", err) | ||
} | ||
|
||
for _, m := range res.Modules { | ||
|
@@ -164,9 +165,32 @@ func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[strin | |
mainModules[res.FileName].Add(m) | ||
} | ||
} | ||
|
||
// Collect all annotations from each file into a single annotations struct. | ||
for k, v := range annotations.ignore { | ||
allAnnotations.ignore[k] = v | ||
} | ||
allAnnotations.includeDep = append(allAnnotations.includeDep, annotations.includeDep...) | ||
} | ||
|
||
return modules, mainModules, nil | ||
allAnnotations.includeDep = removeDupesFromStringTreeSetSlice(allAnnotations.includeDep) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we even want to remove duplicates here? We could just rely on the target builders to handle that. |
||
|
||
return modules, mainModules, allAnnotations, nil | ||
} | ||
|
||
// removeDupesFromStringTreeSetSlice takes a []string, makes a set out of the | ||
// elements, and then returns a new []string with all duplicates removed. Order | ||
// is preserved. | ||
func removeDupesFromStringTreeSetSlice(array []string) []string { | ||
s := treeset.NewWith(godsutils.StringComparator) | ||
for _, v := range array { | ||
s.Add(v) | ||
} | ||
dedupe := make([]string, s.Size()) | ||
for i, v := range s.Values() { | ||
dedupe[i] = fmt.Sprint(v) | ||
} | ||
return dedupe | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Coming from python, this function feels icky to me. I'd really like to be able to just do allAnnotations.includeDep = list(set(allAnnotations.includeDep)) above but I couldn't figure out how to do so. The issue is related to types: the If you have any suggestions on how to clean things up, I'd be happy to hear them. Otherwise no reply is needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been meaning to pick up https://github.com/f0rmiga/datanalgo and add the missing data structures using generics. I can try to resurrect that, which would be ideal to use with the Gazelle extension. |
||
} | ||
|
||
// parserResponse represents a response returned by the parser.py for a given | ||
|
@@ -211,7 +235,8 @@ const ( | |
// The Gazelle annotation prefix. | ||
annotationPrefix string = "gazelle:" | ||
// The ignore annotation kind. E.g. '# gazelle:ignore <module_name>'. | ||
annotationKindIgnore annotationKind = "ignore" | ||
annotationKindIgnore annotationKind = "ignore" | ||
annotationKindIncludeDep annotationKind = "include_dep" | ||
) | ||
|
||
// comment represents a Python comment. | ||
|
@@ -247,12 +272,15 @@ type annotation struct { | |
type annotations struct { | ||
// The parsed modules to be ignored by Gazelle. | ||
ignore map[string]struct{} | ||
// Labels that Gazelle should include as deps of the generated target. | ||
includeDep []string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be plural? Same elsewhere. I originally went singular to be consistent with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd make it plural because it is a list. I don't see a need to make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated 👍 |
||
} | ||
|
||
// annotationsFromComments returns all the annotations parsed out of the | ||
// comments of a Python module. | ||
func annotationsFromComments(comments []comment) (*annotations, error) { | ||
ignore := make(map[string]struct{}) | ||
includeDep := []string{} | ||
for _, comment := range comments { | ||
annotation, err := comment.asAnnotation() | ||
if err != nil { | ||
|
@@ -269,10 +297,21 @@ func annotationsFromComments(comments []comment) (*annotations, error) { | |
ignore[m] = struct{}{} | ||
} | ||
} | ||
if annotation.kind == annotationKindIncludeDep { | ||
targets := strings.Split(annotation.value, ",") | ||
for _, t := range targets { | ||
if t == "" { | ||
continue | ||
} | ||
t = strings.TrimSpace(t) | ||
includeDep = append(includeDep, t) | ||
} | ||
} | ||
} | ||
} | ||
return &annotations{ | ||
ignore: ignore, | ||
ignore: ignore, | ||
includeDep: includeDep, | ||
}, nil | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
# gazelle:python_generation_mode file |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test") | ||
|
||
# gazelle:python_generation_mode file | ||
|
||
py_library( | ||
name = "__init__", | ||
srcs = ["__init__.py"], | ||
visibility = ["//:__subpackages__"], | ||
deps = [ | ||
":module1", | ||
":module2", | ||
"//foo/bar:baz", | ||
"//hello:world", | ||
"@gazelle_python_test//foo", | ||
"@star_wars//rebel_alliance/luke:skywalker", | ||
], | ||
) | ||
|
||
py_library( | ||
name = "module1", | ||
srcs = ["module1.py"], | ||
visibility = ["//:__subpackages__"], | ||
) | ||
|
||
py_library( | ||
name = "module2", | ||
srcs = ["module2.py"], | ||
visibility = ["//:__subpackages__"], | ||
deps = [ | ||
"//checking/py_binary/from/if:works", | ||
"//foo:bar", | ||
], | ||
) | ||
|
||
py_binary( | ||
name = "annotation_include_dep_bin", | ||
srcs = ["__main__.py"], | ||
main = "__main__.py", | ||
visibility = ["//:__subpackages__"], | ||
deps = [ | ||
":module2", | ||
"//checking/py_binary/from/__main__:works", | ||
], | ||
) | ||
|
||
py_test( | ||
name = "module2_test", | ||
srcs = ["module2_test.py"], | ||
deps = [ | ||
":module2", | ||
"//checking/py_test/works:too", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
# Annotation: Include Dep | ||
|
||
Test that the Python gazelle annotation `# gazelle:include_dep` correctly adds dependences | ||
to the generated target even if those dependencies are not imported by the Python module. | ||
|
||
The root directory tests that all `py_*` targets will correctly include the additional | ||
dependencies. | ||
|
||
The `subpkg` directory tests that all `# gazlle:include_dep` annotations found in all source | ||
files are included in the generated target (such as during `generation_mode package`). |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import module1 | ||
import foo # third party package | ||
|
||
# gazelle:include_dep //foo/bar:baz | ||
# gazelle:include_dep //hello:world,@star_wars//rebel_alliance/luke:skywalker | ||
# gazelle:include_dep :module2 | ||
|
||
del module1 | ||
del foo |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# gazelle:include_dep //checking/py_binary/from/__main__:works | ||
# Check deduping | ||
# gazelle:include_dep //checking/py_binary/from/__main__:works | ||
|
||
import module2 | ||
|
||
del module2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Copyright 2023 The Bazel Authors. All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
manifest: | ||
modules_mapping: | ||
foo: foo | ||
pip_deps_repository_name: gazelle_python_test |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# gazelle:include_dep //foo:bar | ||
|
||
if __name__ == "__main__": | ||
# gazelle:include_dep //checking/py_binary/from/if:works | ||
print("hello") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# gazelle:include_dep //checking/py_test/works:too | ||
|
||
import module2 | ||
|
||
del module2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
# gazelle:python_generation_mode package |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
load("@rules_python//python:defs.bzl", "py_library", "py_test") | ||
|
||
# gazelle:python_generation_mode package | ||
|
||
py_library( | ||
name = "subpkg", | ||
srcs = [ | ||
"__init__.py", | ||
"module1.py", | ||
"module2.py", | ||
"module3.py", | ||
], | ||
visibility = ["//:__subpackages__"], | ||
deps = [ | ||
":nonexistant_target_from_include_dep_in_module3", | ||
"//me_from_module1", | ||
"//other/thing:from_include_dep_in_module2", | ||
"//you_from_module1", | ||
], | ||
) | ||
|
||
py_test( | ||
name = "module1_test", | ||
srcs = ["module1_test.py"], | ||
deps = [ | ||
":subpkg", | ||
"//:bagel_from_include_dep_in_module1_test", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
def hello(): | ||
# gazelle:include_dep //you_from_module1,//me_from_module1 | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# gazelle:include_dep //:bagel_from_include_dep_in_module1_test | ||
|
||
import module1 | ||
|
||
del module1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# gazelle:include_dep //other/thing:from_include_dep_in_module2 | ||
import module1 | ||
|
||
del module1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
def goodbye(): | ||
# gazelle:include_dep :nonexistant_target_from_include_dep_in_module3 | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annotations.ignore
is not used ingenerate.go
so we could leave this out if wanted. Thoughts? Leaving it out would mean that the returnedallAnnotations
is not representative of everything that was parsed.