refactor: fix static analysis errors #114
Conversation
| push: | ||
| branches: [ master ] | ||
| pull_request: | ||
| branches: [ master ] |
There was a problem hiding this comment.
Removed this rule, so it allows to run this CI workflow on PRs that don't target master.
|
|
||
| try: | ||
| import ssl | ||
| import ssl # noqa: F401 |
There was a problem hiding this comment.
F401 = unused import
There was a problem hiding this comment.
Add as a comment to cover for documentation ^
| import sys | ||
| import logging | ||
| import os |
There was a problem hiding this comment.
The static analysis tool enforces one import per line
|
|
||
| try: | ||
| foo = "bar" | ||
| foo = "bar" # noqa: F841 |
There was a problem hiding this comment.
F841 = unused variable
There was a problem hiding this comment.
Remove this in that case or add as comment for documentation?
| except Exception: | ||
| result = client.send_exception(httpRequest={}) | ||
|
|
||
| self.assertEqual(result[0], 202) |
There was a problem hiding this comment.
This assert was missing in the original test
|
|
||
|
|
||
| class TestRaygunErrorMessage(unittest.TestCase): | ||
| class TestRaygunErrorMessageChained(unittest.TestCase): |
There was a problem hiding this comment.
This class name was duplicated, which caused that the above tests were ignored. This was detected thanks to the static analysis tool.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to fix static analysis errors reported by flake8. The changes primarily update exception handling to be explicit (using "except Exception:" instead of bare except clauses), remove unused imports, and add noqa comments for unused variable assignments.
- Refactored exception handling in tests and production code.
- Removed unused imports and added "# noqa" annotations where needed.
- Updated CI workflow to re-enable flake8 linting.
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| python3/tests/test_utilities.py | Removed unnecessary imports to satisfy flake8. |
| python3/tests/test_raygunprovider.py | Changed exception handling to be explicit. |
| python3/tests/test_raygunmsgs.py | Updated exception handling and renamed test class for chaining. |
| python3/tests/test_functional.py | Replaced bare except clauses and added noqa comments for unused assignments. |
| python3/samples/sample.py | Updated exception handling to be explicit. |
| python3/raygun4py/raygunprovider.py | Added "# noqa: F401" to the ssl import to address static analysis. |
| python3/raygun4py/raygunmsgs.py | Refactored exception handling and string formatting in warning messages. |
| python3/raygun4py/init.py | Added "# noqa: F401" to suppress unused import warnings. |
| .github/workflows/python-checks.yml | Re-enabled flake8 linting by uncommenting the linting step. |
Files not reviewed (1)
- .flake8: Language not supported
Comments suppressed due to low confidence (1)
python3/tests/test_raygunmsgs.py:240
- The variable 'msg_clone' is used in place of 'msg', but it does not appear to be defined in this context. Verify if 'msg_clone' was intended or if this should be reverted to 'msg'.
self.find_local_variable(msg_clone.stackTrace, "localReference"),
sumitramanga
left a comment
sumitramanga
left a comment
There was a problem hiding this comment.
Looks good, nothing major stands out here 😃
Description 📝
Purpose:
This PR fixes all the static analysis errors found using
flake8.Approach:
Several runs running
flake8and the unit tests to ensure all works correctly.During the process, I found out that some tests were not executed because a test class name was duplicated. The static analysis tool helped find that.
Type of change
fix:Bug fix (non-breaking change which fixes an issue)feat:New feature (non-breaking change which adds functionality)chore:Chore task, release or small impact changeci:CI configuration changeUpdates
Most static analysis errors are fixed, some are ignored because they are necessary for clarity or for the provider to work.
Some tests required fixes as well.
Related issues
Continues work in #112
Test plan 🧪
Author to check 👓
Reviewer to check ✔️