Skip to content
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

Introduce mypy, ruff, and pre-commit #1215

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

corrylc
Copy link
Contributor

@corrylc corrylc commented Apr 22, 2024

Background

When writing detections for Panther, I would like to use as much python type hinting as possible, while also reducing the bugs that I run into. To enable this, mypy and ruff allow for extensive code quality and type hinting checks.

I am open to breaking this PR up if the ruff/pre-commit portions are not acceptable upstream. While I think they would help with panther-analysis code quality, I don't want them to cause the type hinting improvements to be rejected.

In my experience ruff has totally supplanted pylint/flake8 as the best linting tool for Python as it is a superset of nearly all other tools, and is also radically faster.

Changes

  • Applied mypy to panther_base_helpers.py and directly adjacent code until mypy was error free
  • Applied ruff to panther_base_helpers.py and directly adjacent code, then modified ruleset to align to Panther code patterns
  • Applied ruff-format to panther_base_helpers.py and directly adjacent code
  • Introduced pre-commit to run the above tools on the specific files that have been improved
  • Added pre-commit to GitHub Actions to enforce the improvements

Questions

  • Panther has used inconsistent type hints in many locations, using dict, Mapping, and PantherEvent (rarely) as hints for the event object that gets passed around. While dict is not really appropriate in many cases, it isn't always clear whether Mapping or PantherEvent is the best choice. Open to thoughts here.
  • ruff-format is meant to be equivalent to black which is already in use in this codebase. Y'all may want to switch to it in time, since it is much faster, or remove it from this commit if you prefer using black everywhere.
  • mypy is not happy that python-core (and to a lesser degree panther_detection_helpers) is not marked as having types. I would be happy to address that, but the repos are not public. Typing those libraries are the best next step.

@corrylc corrylc requested review from a team April 22, 2024 21:04
@arielkr256
Copy link
Contributor

Hey @corrylc sorry for the delay on this! At this point ruff and mypy are core elements of pypanther. I don't think it makes sense to add mypy to panther-analysis, but adding a ruff pre-commit makes a lot of sense. Would you mind updating this to just add ruff?

@arielkr256 arielkr256 added the github_actions Pull requests that update GitHub Actions code label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants