-
Notifications
You must be signed in to change notification settings - Fork 102
Python plugin implementation #781
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
base: master
Are you sure you want to change the base?
Conversation
@wbqpk3 Nice and large 😅 work, I will try to find some time to review it. Meanwhile please rebase it to |
@mcserep Rebased to master. |
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.
Copilot reviewed 40 out of 46 changed files in this pull request and generated 5 comments.
Files not reviewed (6)
- Config.cmake: Language not supported
- plugins/python/CMakeLists.txt: Language not supported
- plugins/python/model/CMakeLists.txt: Language not supported
- plugins/python/parser/.gitignore: Language not supported
- plugins/python/parser/CMakeLists.txt: Language not supported
- plugins/python/parser/requirements.txt: Language not supported
except: | ||
if self.config.debug: | ||
log(f"{bcolors.FAIL}Failed to find definition! (file = {str(name.module_path)} line = {name.line} column = {name.column})") | ||
if self.config.stack_trace: |
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.
Avoid using a bare except; replace 'except:' with 'except Exception as e:' to prevent unintentionally catching system-exiting exceptions.
except: | |
if self.config.debug: | |
log(f"{bcolors.FAIL}Failed to find definition! (file = {str(name.module_path)} line = {name.line} column = {name.column})") | |
if self.config.stack_trace: | |
except Exception as e: | |
if self.config.debug: | |
log(f"{bcolors.FAIL}Failed to find definition! (file = {str(name.module_path)} line = {name.line} column = {name.column}) Exception: {e}") |
Copilot uses AI. Check for mistakes.
except: | ||
if self.config.debug: | ||
log(f"{bcolors.FAIL}Failed to find references! (file = {str(x.module_path)} line = {x.line} column = {x.column})") | ||
if self.config.stack_trace: |
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.
Avoid a bare except block here; use 'except Exception as e:' to ensure only anticipated exceptions are caught.
except: | |
if self.config.debug: | |
log(f"{bcolors.FAIL}Failed to find references! (file = {str(x.module_path)} line = {x.line} column = {x.column})") | |
if self.config.stack_trace: | |
except Exception as e: | |
if self.config.debug: | |
log(f"{bcolors.FAIL}Failed to find references! (file = {str(x.module_path)} line = {x.line} column = {x.column}) Exception: {str(e)}") |
Copilot uses AI. Check for mistakes.
from dataclasses import dataclass | ||
|
||
@dataclass | ||
class ParseResult: | ||
path: str | ||
status: str = "full" | ||
nodes = [] | ||
imports = [] |
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.
Using a mutable default (an empty list) in a dataclass can lead to unexpected side effects; consider using field(default_factory=list).
from dataclasses import dataclass | |
@dataclass | |
class ParseResult: | |
path: str | |
status: str = "full" | |
nodes = [] | |
imports = [] | |
from dataclasses import dataclass, field | |
@dataclass | |
class ParseResult: | |
path: str | |
status: str = "full" | |
nodes: list = field(default_factory=list) | |
imports: list = field(default_factory=list) |
Copilot uses AI. Check for mistakes.
from dataclasses import dataclass | ||
|
||
@dataclass | ||
class ParseResult: | ||
path: str | ||
status: str = "full" | ||
nodes = [] | ||
imports = [] |
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.
Avoid using a mutable default value by replacing the direct assignment with field(default_factory=list) to ensure instance isolation.
from dataclasses import dataclass | |
@dataclass | |
class ParseResult: | |
path: str | |
status: str = "full" | |
nodes = [] | |
imports = [] | |
from dataclasses import dataclass, field | |
@dataclass | |
class ParseResult: | |
path: str | |
status: str = "full" | |
nodes: list = field(default_factory=list) | |
imports: list = field(default_factory=list) |
Copilot uses AI. Check for mistakes.
try: | ||
tree = ast.parse(source) | ||
self.astNodes = list(ast.walk(tree)) | ||
except: |
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.
Replace the bare except with 'except Exception:' to catch only the expected exceptions during AST parsing.
except: | |
except Exception: |
Copilot uses AI. Check for mistakes.
@wbqpk3 I just tried out this Copilot-based review feature of GitHub. The suggestions maybe worth a check, but feel free to ignore them in case they look unimportant. |
@wbqpk3 The CI fails for Ubuntu 20.04. While we should drop support for Ubuntu 20.04, it should still be checked why the build fails there.
|
UPDATE: CI jobs for Ubuntu 20.04 already started to skip, due to image unavailability. I have disabled the CI build for Ubuntu 20.04 in b9e6733, and added an issue #789 to deprecate it completely. Fixing this CI fail is not required. |
This PR implements the Python plugin for CodeCompass.
Plugin components:
Parser:
pyparser
written in Python, uses the Jedi static analysis library. Moreover,pyparser
also parses the Python AST in order to achieve better results.pyparser
module gets loaded using the Python C APIPYName
Service:
WebGUI:
Tests:
Documentation:
doc/pythonplugin.md
.The missing part is the diagrams in the new WebGUI, however the diagrams component file needs some refactoring to use the new
language-service
instead ofCppService
(similarly to #759).I think we should implement that in a separate PR.