-
Notifications
You must be signed in to change notification settings - Fork 0
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: skip syntax errors #17
Conversation
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.
Hey @bm424 - I've reviewed your changes and they look great!
General suggestions:
- Ensure that the handling of syntax errors is robust and provides enough information for debugging.
- Consider the performance implications of creating multiple instances of AstroidManager.
- Review the added docstrings for clarity and consistency with the project's documentation style.
- Verify that the changes do not introduce any unintended side effects, especially in error handling and logging.
- Assess the impact of these changes on the overall architecture and maintainability of the codebase.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue found
- 🟡 Docstrings: 8 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -107,6 +110,7 @@ class Extractor(typing.Generic[T]): | |||
""" | |||
|
|||
visitor: Visitor[typing.Optional[T]] = IdentityVisitor() | |||
manager: astroid.manager.AstroidManager = astroid.manager.AstroidManager() |
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.
suggestion (performance): Consider using a singleton for AstroidManager.
Instantiating AstroidManager directly in the Extractor class might lead to unnecessary memory usage and initialization overhead if multiple Extractor instances are created. AstroidManager is typically used as a singleton, consider accessing it via a class method or a module-level instance.
manager: astroid.manager.AstroidManager = astroid.manager.AstroidManager() | |
manager: astroid.manager.AstroidManager = astroid.manager.AstroidManager() | |
+ manager: astroid.manager.AstroidManager = astroid.manager.AstroidManager() |
except astroid.AstroidSyntaxError as e: | ||
if isinstance(e.error, SyntaxError): | ||
error_message = _format_syntax_error_message( | ||
"skipping file", file, e.error | ||
) | ||
else: | ||
error_message = str(e).replace("\n", " ") | ||
warning = SyntaxWarning(error_message) | ||
warnings.warn(warning) | ||
yield from () |
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.
suggestion (code_refinement): Ensure proper handling of syntax errors for robustness.
The addition of syntax error handling in _extract_from_file
is a good improvement for robustness. It might be worth considering if there are any specific actions or logging that should occur when a syntax error is encountered, beyond just warning and skipping the file, to aid in debugging or further analysis.
except astroid.AstroidSyntaxError as e: | |
if isinstance(e.error, SyntaxError): | |
error_message = _format_syntax_error_message( | |
"skipping file", file, e.error | |
) | |
else: | |
error_message = str(e).replace("\n", " ") | |
warning = SyntaxWarning(error_message) | |
warnings.warn(warning) | |
yield from () | |
except astroid.AstroidSyntaxError as e: | |
if isinstance(e.error, SyntaxError): | |
error_message = _format_syntax_error_message( | |
"Syntax error encountered, skipping file", file, e.error | |
) | |
# Log the syntax error for further analysis | |
logging.error(f"Syntax error in file {file}: {e.error}") | |
else: | |
error_message = str(e).replace("\n", " ") | |
# Log generic astroid errors | |
logging.error(f"Astroid error in file {file}: {error_message}") | |
warning = SyntaxWarning(error_message) | |
warnings.warn(warning) | |
# Optionally, you could add more specific handling here, such as: | |
# - Saving the file path and error details to a report for later analysis | |
# - Attempting to auto-correct known syntax issues and reprocess the file | |
# - Emitting metrics or alerts for monitoring the frequency of syntax errors | |
yield from () |
@@ -161,8 +163,7 @@ def cli_assess( | |||
console.print(f"[bold red]Found {count} errors.") | |||
raise typer.Exit(1) | |||
|
|||
console.print("[bold green]Assessment Complete") | |||
console.print("[green]No issues found.") | |||
console.print("[bold green]Assessment Complete", "[green]No issues found.") |
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.
nitpick (code_refinement): Combine console print statements for cleaner output.
Combining the two messages into a single console.print
call in cli_assess
is a neat way to streamline the output. However, ensure that the intended formatting is preserved, as this change might concatenate the messages without the intended newline or separation.
console.print("[bold green]Assessment Complete", "[green]No issues found.") | |
console.print("[bold green]Assessment Complete\n[green]No issues found.") |
""", | ||
], | ||
) | ||
def test_extract_with_syntax_error(self, extractor, file_path, file, caplog): |
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.
suggestion (testing): Consider adding a test case for handling multiple syntax errors in a single file.
This test currently covers the scenario where a file with a syntax error is processed. It would be beneficial to also test how the extractor behaves when multiple syntax errors are present in a single file, ensuring that all errors are appropriately logged or warned.
""", | ||
], | ||
) | ||
def test_extract_with_syntax_error(self, extractor, file_path, file, caplog): |
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.
suggestion (testing): Test case for syntax error does not verify the content of the warning message.
While the test ensures a warning is raised when encountering a syntax error, it does not verify the content of the warning message. It's important to check that the warning message accurately reflects the error encountered, including the file path and the specific syntax error message.
return method.lineno | ||
|
||
|
||
@nodedispatch | ||
@validate_node_type(astroid.nodes.FunctionDef) | ||
def method_file(method: astroid.nodes.FunctionDef) -> str: | ||
"""Returns the file name the method is in. |
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.
suggestion (docstrings): Please update the docstring for function: method_file
Reason for update: Addition of examples and a more detailed description.
Suggested new docstring:
"""Returns the file name the method is in.
Not very useful by itself, but can be combined with other metrics for convenience.
Examples:
Inline code doesn't have a file.
>>> method_file("def foo(): pass")
'<>'
"""
thresholds: ThresholdSettings = ThresholdSettings() | ||
|
||
@classmethod | ||
def from_toml_file(cls, toml_file_path: pathlib.Path): | ||
"""Construct settings from a toml file. |
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.
suggestion (docstrings): Please update the docstring for function: from_toml_file
Reason for update: Addition of detailed argument and return type descriptions.
Suggested new docstring:
"""Construct settings from a toml file.
Args:
toml_file_path:
Relative or fully-qualified path to a toml file containing
sourcery-analytics settings.
Returns:
A Settings instance.
"""
@@ -99,6 +99,11 @@ def aggregate_csv_output(aggregation_method, method_metric, methods, metrics): | |||
|
|||
|
|||
def read_settings(settings_file: pathlib.Path, console: Console) -> Settings: | |||
"""Loads settings in the CLI. |
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.
suggestion (docstrings): Please update the docstring for function: read_settings
Reason for update: Clarification of the function's purpose and behavior.
Suggested new docstring:
"""Loads settings in the CLI.
Wraps the basic settings loader in order to print relevant error messages and
exit with correct codes.
"""
@@ -19,7 +22,8 @@ def from_dict( | |||
cls, | |||
d: typing.Dict[str, typing.Any], | |||
threshold_settings: ThresholdSettings, | |||
): | |||
) -> "ThresholdBreach": |
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.
suggestion (docstrings): Please update the docstring for function: from_dict
Reason for update: Clarification of the construction process from a dictionary.
Suggested new docstring:
"""Constructs a ThresholdBreach instance from a dictionary.
"""
@@ -38,12 +38,30 @@ def method_name(method: astroid.nodes.FunctionDef) -> str: | |||
@nodedispatch | |||
@validate_node_type(astroid.nodes.FunctionDef) | |||
def method_lineno(method: astroid.nodes.FunctionDef) -> int: | |||
"""Returns the line number of the method. | |||
|
|||
Not very useful by itself, but can be combined with other metrics for convenience. |
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.
issue (complexity): Consider simplifying the implementation for consistency and readability.
While the added functionality is appreciated, I suggest simplifying the implementation to maintain consistency and readability across the codebase. For instance, the extensive docstrings for relatively straightforward functions like method_lineno
and method_file
could be made more concise. Additionally, the nested function definition within method_file
adds unnecessary complexity. Consider integrating the logic directly into the method_file
function to streamline the flow and improve readability. Simplifying these aspects would make the codebase easier to maintain and understand.
Directory processing will skip files containing syntax errors with a warning.