Added support for python scripts. #1415#1419
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Sorry for the slow response! This looks like it would open and read every file in a directory and subdirectories to find Python scripts? That seems like it would be too slow to be practical, unfortunately. What do you think of only allowing directly specified files to have non-.py extensions? That is, if you run |
|
@rchen152. That makes sense. I'll get on it right away. |
|
@rchen152, have added another commit to only check for python scripts when a non- |
rchen152
left a comment
rchen152
left a comment
There was a problem hiding this comment.
Thanks! A few comments.
| if path_utils.isfile(file_path): | ||
| with open(file_path, 'r') as file: | ||
| line = file.readline().rstrip().lower() | ||
| return line.startswith('#!') and (line.endswith('python') or line.endswith('python3')) |
There was a problem hiding this comment.
I'd recommend using a regex here. Something like return re.fullmatch(r'#!.+python3?', line) is not None should work.
| # If we have a directory, collect all the .py files within it. | ||
| out += recursive_glob(path_utils.join(f, "**", "*.py")) | ||
| # If we have a directory, collect all the .py files within it.... | ||
| out += recursive_glob(path_utils.join(f, "**", "*.py")) |
There was a problem hiding this comment.
| out += recursive_glob(path_utils.join(f, "**", "*.py")) | |
| out += recursive_glob(path_utils.join(f, "**", "*.py")) |
| def _test_expand(self, string): | ||
| with test_utils.Tempdir() as d: | ||
| fs = [d.create_file(f) for f in self.FILES] | ||
| fs += [d.create_file("my_script", self.SCRIPT_CODE)] |
There was a problem hiding this comment.
What is this line accomplishing? The test still asserts that expand_source_files only finds the .py files.
There was a problem hiding this comment.
Previously when I was checking every file in the directory to check if it was a script, this line would have made sense to make sure we don't miss a script file. Now that we only check for a file that is passed to pytype, this line looks redundant.
I will remove it. Thanks for pointing that out.
|
@rchen152, added changes as requested. Hope that is good. |
rchen152
left a comment
rchen152
left a comment
There was a problem hiding this comment.
Looking good! There are a couple of lint errors: https://github.com/google/pytype/actions/runs/4945740447/jobs/8861864375
- There's extra whitespace on line 144.
- The parameter name
diron line 145 conflicts with thedirbuiltin.
Once those are fixed, I think this will be ready to merge.
|
@rchen152, Done!!!!!!!! |
rchen152
left a comment
rchen152
left a comment
There was a problem hiding this comment.
Thanks for seeing this through!
…1415 I merged the PR directly - it honestly just seems like a better experience for external contributors. But we still need to copy the changes over so they don't get overwritten. PiperOrigin-RevId: 531600356
2 participants
Regarding issue #1415, I have added support for Python scripts by checking for a shebang at the start of the file with the path to the Python executable.
One thing to note with this approach is that it will check all files in the directory/those that you provide to see if they are python executable scripts. That is, even if you have a file called
main.html, but it has a python shebang line at the top and has python code, then it will be type checked.This works for me, but I am also looking forward to any opinions that could provide more robust solutions.