Codesearch: Add a general-purpose Python extractor. [infra/infra : main]
Alex Ovsienko (Gerrit)
Alex Ovsienko added 1 comment![]( "Open in Gerrit")
Related details
- Marc Jin
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ovsienko (Gerrit)
Alex Ovsienko added 3 comments![]( "Open in Gerrit")
def build_compilation_unit_json(target, deps, project_root, corpus):As it's a new file, let's have proper documenattion and type hints.
elif "contents" + os.sep + "lib" in abs_path:How about f strings instead, it's using python 3.11 right?
Related details
- Marc Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Jin (Gerrit)
Marc Jin added 1 comment![]( "Open in Gerrit")
Alex, need your opinion on this -
I plan to run this script in the recipes using something like
`vpython3 python_extractor.py target_dir=[cache_dir]/chrome_infra/infra --root=[cache_dir]/chrome_infra --corpus=chromium.googlesource.com/infra/infra_superproject//main --output_json=[cache_dir]/chrome_infra/infra.json`
Run this through all the submodules, and then use a go binary to build kzips against these json files.
Related details
- Marc Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ovsienko (Gerrit)
Alex Ovsienko added 1 comment![]( "Open in Gerrit")
Alex, need your opinion on this -
I plan to run this script in the recipes using something like
`vpython3 python_extractor.py target_dir=[cache_dir]/chrome_infra/infra --root=[cache_dir]/chrome_infra --corpus=chromium.googlesource.com/infra/infra_superproject//main --output_json=[cache_dir]/chrome_infra/infra.json`Run this through all the submodules, and then use a go binary to build kzips against these json files.
Doesn't seem like it should be an issue, I can't say much about what you are trying to do here, but vpython3 should pass over the parameters to python.
Related details
- Marc Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Jin (Gerrit)
Marc Jin added 4 comments![]( "Open in Gerrit")
Can we have unit tests?
Added
def build_compilation_unit_json(target, deps, project_root, corpus):As it's a new file, let's have proper documenattion and type hints.
Done
How about f strings instead, it's using python 3.11 right?
- Alex Ovsienko
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ovsienko (Gerrit)
Alex Ovsienko added 1 comment![]( "Open in Gerrit")
def build_compilation_unit_json(target, deps, project_root, corpus):Marc JinAs it's a new file, let's have proper documenattion and type hints.
Done
Yeah but I also meant like `target: str, deps: list[str] and such. Gemini should figure those out.`
Related details
- Marc Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ovsienko (Gerrit)
Alex Ovsienko added 2 comments![]( "Open in Gerrit")
target_corpus = 'python_stdlib'Maybe make this default above everything, and do the same with rel_path = abs_path.lstrip(os.sep)?
return filename.endswith('.py')I would disagree, I can point at plenty of executable pythons that don't have .py in the end. Will we support them later?
Related details
- Marc Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ovsienko (Gerrit)
Alex Ovsienko added 4 comments![]( "Open in Gerrit")
external_search_dirs = [
d for d in sys.path
if d and os.path.isdir(d) and not d.startswith(root_dir)
]I like lambdas but this one is a bit too much. Maybe use filter instead?
for candidate in repo_files_by_name.get(last_comp, []):The default is redundant, either get by [] or remove defaultdict(list)
for filepath in all_py_files:
deps = direct_deps[filepath]
transitive_deps = get_transitive_deps(filepath, direct_deps)
if output_json:
cu = build_compilation_unit_json(filepath, transitive_deps, project_root,
corpus)
all_units.append(cu)
# Dump to JSON format as CompilationUnit Proto
if output_json:
units_dict_list = [
json_format.MessageToDict(cu, preserving_proto_field_name=True)
for cu in all_units
]
os.makedirs(os.path.dirname(os.path.abspath(output_json)), exist_ok=True)
with open(output_json, 'w', encoding='utf-8') as f:
json.dump(units_dict_list, f, indent=2)Isn't this entire block moot if output_json is empty? Maybe early quit instead?
output_json = os.path.abspath(args.output_json) if args.output_json else NoneMaybe empty string instead of optional? Your choice though.
Related details
- Marc Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Jin (Gerrit)
Marc Jin added 7 comments![]( "Open in Gerrit")
def build_compilation_unit_json(target, deps, project_root, corpus):Marc JinAs it's a new file, let's have proper documenattion and type hints.
Alex OvsienkoDone
Yeah but I also meant like `target: str, deps: list[str] and such. Gemini should figure those out.`
Done
Maybe make this default above everything, and do the same with rel_path = abs_path.lstrip(os.sep)?
I updated this, but I think the default should be the if-case
I would disagree, I can point at plenty of executable pythons that don't have .py in the end. Will we support them later?
Does the current shebang check way look better?
external_search_dirs = [
d for d in sys.path
if d and os.path.isdir(d) and not d.startswith(root_dir)
]I like lambdas but this one is a bit too much. Maybe use filter instead?
Using filter() is also using lambda functions...
external_search_dirs = list(
filter(
lambda d: d and os.path.isdir(d) and not d.startswith(root_dir),
sys.path))
This doesn't look much different?
for candidate in repo_files_by_name.get(last_comp, []):The default is redundant, either get by [] or remove defaultdict(list)
Good catch thank you!
for filepath in all_py_files:
deps = direct_deps[filepath]
transitive_deps = get_transitive_deps(filepath, direct_deps)
if output_json:
cu = build_compilation_unit_json(filepath, transitive_deps, project_root,
corpus)
all_units.append(cu)
# Dump to JSON format as CompilationUnit Proto
if output_json:
units_dict_list = [
json_format.MessageToDict(cu, preserving_proto_field_name=True)
for cu in all_units
]
os.makedirs(os.path.dirname(os.path.abspath(output_json)), exist_ok=True)
with open(output_json, 'w', encoding='utf-8') as f:
json.dump(units_dict_list, f, indent=2)Isn't this entire block moot if output_json is empty? Maybe early quit instead?
after a 2nd thought I think we should enforce output_json path otherwise this extractor is totally useless
output_json = os.path.abspath(args.output_json) if args.output_json else NoneMaybe empty string instead of optional? Your choice though.
- Alex Ovsienko
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ovsienko (Gerrit)
Alex Ovsienko added 2 comments![]( "Open in Gerrit")
return filename.endswith('.py')Marc JinI would disagree, I can point at plenty of executable pythons that don't have .py in the end. Will we support them later?
Does the current shebang check way look better?
Yep, check depot_tools's yapf as an example. However .py would likely not add shebang, so you need to check for both
external_search_dirs = [
d for d in sys.path
if d and os.path.isdir(d) and not d.startswith(root_dir)
]Marc JinI like lambdas but this one is a bit too much. Maybe use filter instead?
Using filter() is also using lambda functions...
external_search_dirs = list(
filter(
lambda d: d and os.path.isdir(d) and not d.startswith(root_dir),
sys.path))
This doesn't look much different?
- Marc Jin
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ovsienko (Gerrit)
Alex Ovsienko added 1 comment![]( "Open in Gerrit")
external_search_dirs = [
d for d in sys.path
if d and os.path.isdir(d) and not d.startswith(root_dir)
]Marc JinI like lambdas but this one is a bit too much. Maybe use filter instead?
Alex OvsienkoUsing filter() is also using lambda functions...
external_search_dirs = list(
filter(
lambda d: d and os.path.isdir(d) and not d.startswith(root_dir),
sys.path))
This doesn't look much different?
Then I guess just loop over sys.path?
Actually nevermind, it's fine as is in the end.
Related details
- Marc Jin
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Jin (Gerrit)
Marc Jin added 1 comment![]( "Open in Gerrit")
- Fumitoshi Ukai
- Gavin Mak
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ovsienko (Gerrit)
Alex Ovsienko voted Code-Review+1![]( "Open in Gerrit")
- Fumitoshi Ukai
- Gavin Mak
- Marc Jin
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |