-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add --exclude-location flag to hide file locations in detector messages #2872
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
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.
Code Review
Thanks for this PR - the feature addresses a real need from issue #2222.
Main Concern: Global Variable Pattern
The implementation uses a module-level global:
_exclude_location = False
def set_exclude_location(exclude: bool) -> None:
global _exclude_location
_exclude_location = excludeThis works, but the codebase already has a more idiomatic pattern in colors.py:
class Colors:
COLORIZATION_ENABLED = True
def set_colorization_enabled(enabled: bool) -> None:
Colors.COLORIZATION_ENABLED = enabledSuggestion: Use a class attribute for consistency:
class OutputConfig:
EXCLUDE_LOCATION = False
def set_exclude_location(enabled: bool) -> None:
OutputConfig.EXCLUDE_LOCATION = enabled
def get_exclude_location() -> bool:
return OutputConfig.EXCLUDE_LOCATIONThis matches the existing pattern and makes the configuration more discoverable.
Minor: Repetitive if/else in _convert_to_description
The repeated pattern could be simplified with a helper:
def _format_with_location(name: str, source_mapping, exclude: bool) -> str:
return name if exclude else f"{name} ({source_mapping})"Overall the feature is solid. Just requesting the pattern change for consistency with the rest of the codebase.
d878444 to
81e51a9
Compare
|
Hi maintainers, could someone please approve the CI workflow runs when you get a chance? This PR is ready for review. Thanks! |
ep0chzer0
left a comment
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.
Thanks for the detailed review! I believe the current implementation already addresses both points:
1. Class Attribute Pattern
The code already uses the OutputConfig class pattern (matching colors.py):
class OutputConfig:
"""Configuration settings for output formatting."""
EXCLUDE_LOCATION: bool = False
def set_exclude_location(exclude: bool) -> None:
OutputConfig.EXCLUDE_LOCATION = exclude
def get_exclude_location() -> bool:
return OutputConfig.EXCLUDE_LOCATION2. Helper Function
The repetitive if/else pattern is already simplified with the _format_with_location helper:
def _format_with_location(name: str, source_mapping: SourceMapping) -> str:
if OutputConfig.EXCLUDE_LOCATION:
return name
return f"{name} ({source_mapping})"This is used throughout _convert_to_description() to reduce repetition.
Could you take another look at the current diff? Perhaps there was a caching issue or you were viewing an earlier version. Happy to make any additional changes if needed!
2bd13d3 to
71c27e0
Compare
This adds a new CLI flag `--exclude-location` that allows users to exclude file location information (filename and line numbers) from detector output. This is useful for cleaner output when locations are not needed. Changes: - Add OutputConfig class with EXCLUDE_LOCATION setting (matches colors.py pattern) - Add _format_with_location helper function for consistent formatting - Update _convert_to_description to use helper for conditional location - Add CLI flag --exclude-location in __main__.py - Add default to command_line.py - Add unit tests for the new functionality Also includes inheritance printer fixes (issue crytic#1835): - Remove extra newlines causing malformed output - Fix bug storing `immediate` in `not_immediate` key - Use PEP8 style `child not in` instead of `not child in` Fixes crytic#2222
71c27e0 to
be77d2a
Compare
|
Thanks for the review @dguido! I've implemented both suggestions:
The changes are already in the branch. CI is passing - just waiting for workflow approval on the latest commit after rebase. |
|
Also @dguido, if there are any other specific issues you'd like to see tackled, I'd be happy to take them on. Always looking for ways to contribute to Slither! |
Summary
Fixes #2222
Adds a new
--exclude-locationflag that removes file location information (filename and line numbers) from detector messages.Before/After
Without flag:
With
--exclude-location:Use Cases
As noted in the issue:
Changes
slither/utils/output.py: Addset_exclude_location()andget_exclude_location()functions, modify_convert_to_description()to conditionally exclude locationslither/__main__.py: Add--exclude-locationCLI argument and callset_exclude_location()slither/utils/command_line.py: Add default value forexclude_locationTesting
Tested manually with a simple Solidity file - the location info is correctly excluded when the flag is provided.