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

Don't log errors that we end up raising #22557 #8819

Merged
merged 14 commits into from
Sep 10, 2024

Conversation

JessicaBell00
Copy link
Contributor

Addresses Azure/azure-sdk-for-python#22557
Checks that errors raised in exception blocks aren't also logged at 'warning' and 'error' levels.

Added new rule for this issue to the pylint_guidelines_checker file.
Added corresponding tests to the test_pylint_custom_plugins file
Added two more tests to check for multiple exception blocks
Changed 'warning' to '.warning' etc. Help avoid any false positives e.g. 'print("error")' will now not trigger Pylint
Merge branch 'main' into dont-log-errors-end-up-raising
Also added another test to test_pylint_custom_plugins.py for 'warning' level.
@JessicaBell00
Copy link
Contributor Author

@microsoft-github-policy-service agree

Also rename checker to "do-not-log-raised-errors"
Added '.lower()' to string being parsed for logging.
@l0lawrence
Copy link
Member

/azp run tools - azure-pylint-guidelines-checker - ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@annatisch annatisch left a comment

Choose a reason for hiding this comment

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

Looking good - I think we're nearly there.
Do you have a sample output for SDKs in the repo that get flagged by this new rule?

JessicaBell00 and others added 2 commits September 2, 2024 12:41
…_guidelines_checker.py

Co-authored-by: Anna Tisch <antisch@microsoft.com>
@JessicaBell00
Copy link
Contributor Author

JessicaBell00 commented Sep 2, 2024

Looking good - I think we're nearly there. Do you have a sample output for SDKs in the repo that get flagged by this new rule?

The output of the Pylint testing against the SDKs is below. This was before the changes I have just made. Will run tests again later :). All of them are true positives.
pylintreport.txt

Refactored function and removed 'isInstance Astroid.For' and associated tests
@JessicaBell00
Copy link
Contributor Author

Output from re-testing against the SDKs after changes made yesterday. Looks good, all true positives :)
pylintreport2.txt

@l0lawrence l0lawrence merged commit 0faf143 into Azure:main Sep 10, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants