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

Add flake8 linter & fix conflict between isort and black #653

Merged
merged 3 commits into from
Feb 5, 2025

Conversation

Ana06
Copy link
Member

@Ana06 Ana06 commented Feb 4, 2025

  • Add max length of an import line to isort to avoid conflicts with black.
  • Add flake8 linter to CI to improve code quality and maintainability.
  • Fix flake8 offenses identified by the linter.
  • Standardizee the formatting of multiline strings across all scripts, ensuring consistency and improved readability.

Add max length of an import line to isort to avoid conflicts with black.
@Ana06 Ana06 added the CI Related to CI label Feb 4, 2025
@Ana06 Ana06 requested a review from williballenthin February 4, 2025 15:57
@Ana06 Ana06 self-assigned this Feb 4, 2025
- name: Run isort
run: isort --check --diff .
run: isort --check --diff --line-length 120 .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it 120 here and 150 above?

Copy link
Member Author

@Ana06 Ana06 Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

black does not complain about strings or fix long string, while flake8 does complain. A lower max line in flake8 generates the following offences. I feel fixing this offenses is more annoying that helpful as the strings become more difficult to read.

$ flake8 --max-line-length 120
./virtualbox/vbox-adapter-check.py:97:121: E501 line too long (136 > 120 characters)
./virtualbox/vbox-adapter-check.py:101:121: E501 line too long (128 > 120 characters)
./virtualbox/vbox-adapter-check.py:127:121: E501 line too long (145 > 120 characters)
./virtualbox/vbox-adapter-check.py:131:121: E501 line too long (124 > 120 characters)
./virtualbox/vbox-clean-snapshots.py:24:121: E501 line too long (141 > 120 characters)
./virtualbox/vbox-clean-snapshots.py:29:121: E501 line too long (150 > 120 characters)
./virtualbox/vbox-clean-snapshots.py:32:121: E501 line too long (141 > 120 characters)
./virtualbox/vbox-clean-snapshots.py:35:121: E501 line too long (135 > 120 characters)
./virtualbox/vbox-clean-snapshots.py:38:121: E501 line too long (137 > 120 characters)
./virtualbox/vbox-clean-snapshots.py:58:121: E501 line too long (131 > 120 characters)
./virtualbox/vbox-clean-snapshots.py:114:121: E501 line too long (123 > 120 characters)
./virtualbox/vbox-export-snapshots.py:29:121: E501 line too long (134 > 120 characters)

@williballenthin do you think is it ok to leave these 2 different max and add a clarifying comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't bother me, just wondering. thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @williballenthin!

Ana06 added 2 commits February 4, 2025 17:46
This commit adds flake8 to the continuous integration (CI) pipeline.
flake8 will now run on every push and pull request, helping to enforce
code style consistency and identify potential issues early.  This will
improve code quality and maintainability.
This commit addresses flake8 offenses identified by the linter.  It also
standardizes the formatting of multiline strings across all scripts,
ensuring consistency and improved readability.
@Ana06 Ana06 merged commit f87e636 into mandiant:main Feb 5, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Related to CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants