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

Fix "Exception statement has no effect" #37722

Merged
merged 3 commits into from
Feb 29, 2024

Conversation

Taragolis
Copy link
Contributor

@Taragolis Taragolis commented Feb 26, 2024

During some code review I've accidentally found sometimes we do not raise an error or warning in our code.
Unfortunetly there is no such of check into the ruff, suggested B018 can't detect this cases, so I propose to run this kind of check (W0133) by pylint, and only this one.

pre-commit run pylint --all-files
pylint...................................................................Failed
- hook id: pylint
- exit code: 4

************* Module google.cloud.operators.cloud_storage_transfer_service
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:552:16: W0133: Exception statement has no effect (pointless-exception-statement)
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:556:16: W0133: Exception statement has no effect (pointless-exception-statement)
************* Module google.cloud.transfers.gcs_to_local
airflow/providers/google/cloud/transfers/gcs_to_local.py:94:16: W0133: Exception statement has no effect (pointless-exception-statement)
airflow/providers/google/cloud/transfers/gcs_to_local.py:96:16: W0133: Exception statement has no effect (pointless-exception-statement)
************* Module opensearch.hooks.opensearch
airflow/providers/opensearch/hooks/opensearch.py:106:12: W0133: Exception statement has no effect (pointless-exception-statement

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member

potiuk commented Feb 26, 2024

I was a little sceptical. We removed pylint gladly when we introduced ruff. Rather than introducing new tool for that, I'd generally wait until ruff supports such rule.
But with that very specific rule - I see that "full check" is extremely fast. so yeah, let's do it.

@Taragolis
Copy link
Contributor Author

I'm also not happy to have another linter, but yeah seems like this single rule work pretty fast.

I could open issue/feature request at ruff repo with appropriate example when neither ruff B018 or flake8-bugbear doesn't work. Maybe one day it would be added into the ruff, so we could remove pylint again

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Nice one!

@uranusjr
Copy link
Member

We should open an issue on Ruff for this.

@potiuk
Copy link
Member

potiuk commented Feb 27, 2024

I could open issue/feature request at ruff repo with appropriate example when neither ruff B018 or flake8-bugbear doesn't work. Maybe one day it would be added into the ruff, so we could remove pylint again

We should open an issue on Ruff for this.

Yes. Yes.

@Taragolis
Copy link
Contributor Author

I've just created new rule request at ruff GitHub Issues: astral-sh/ruff#10145

@Taragolis Taragolis changed the title Static Checks: Exception statement has no effect Fix "Exception statement has no effect" Feb 27, 2024
@Taragolis Taragolis removed the provider:google Google (including GCP) related issues label Feb 29, 2024
@Taragolis
Copy link
Contributor Author

Just to be on the same page. We do not have any objections to temporary include pylint into our static check until this rule not implemented into the ruff?

@potiuk
Copy link
Member

potiuk commented Feb 29, 2024

I am fine. It's fast, easy to remove

@potiuk potiuk merged commit a538e02 into apache:main Feb 29, 2024
99 checks passed
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
* Fix "Exception statement has no effect"

* Add link to the ruff issue tracker

* update breeze hashes
@utkarsharma2 utkarsharma2 added this to the Airflow 2.8.3 milestone Mar 6, 2024
@utkarsharma2 utkarsharma2 added the type:misc/internal Changelog: Misc changes that should appear in change log label Mar 6, 2024
ephraimbuddy pushed a commit that referenced this pull request Mar 6, 2024
* Fix "Exception statement has no effect"

* Add link to the ruff issue tracker

* update breeze hashes

(cherry picked from commit a538e02)
@ephraimbuddy ephraimbuddy removed this from the Airflow 2.8.3 milestone Mar 7, 2024
@ephraimbuddy ephraimbuddy added changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) and removed type:misc/internal Changelog: Misc changes that should appear in change log labels Mar 20, 2024
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
* Fix "Exception statement has no effect"

* Add link to the ruff issue tracker

* update breeze hashes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:opensearch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants