Skip to content

Replace to broad exceptions into the Dev#38489

Merged
Taragolis merged 3 commits into
apache:mainfrom
Taragolis:too-broad-exceptions-dev
Apr 4, 2024
Merged

Replace to broad exceptions into the Dev#38489
Taragolis merged 3 commits into
apache:mainfrom
Taragolis:too-broad-exceptions-dev

Conversation

@Taragolis

Copy link
Copy Markdown
Contributor

Similar to: #38344

This one less affect the tests but still better to use custom or other more appropriate builtin exceptions rather than just Exception.

This one was found by TRY002 rule it has a bit confusing message

ruff check dev --select "TRY002"

dev/breeze/src/airflow_breeze/commands/release_management_commands.py:1069:19: TRY002 Create your own exception
dev/breeze/src/airflow_breeze/commands/release_management_commands.py:1094:15: TRY002 Create your own exception
dev/breeze/src/airflow_breeze/commands/release_management_commands.py:1219:19: TRY002 Create your own exception
dev/breeze/src/airflow_breeze/global_constants.py:363:15: TRY002 Create your own exception
dev/breeze/src/airflow_breeze/utils/docs_publisher.py:61:19: TRY002 Create your own exception
dev/breeze/src/airflow_breeze/utils/kubernetes_utils.py:95:11: TRY002 Create your own exception
dev/breeze/src/airflow_breeze/utils/packages.py:136:19: TRY002 Create your own exception
dev/breeze/src/airflow_breeze/utils/packages.py:180:15: TRY002 Create your own exception
Found 8 errors.

We might want to activate this rule later on, just need to resolve it into the providers first (or add noqa):

ruff check airflow/providers --select "TRY002"
...
Found 59 errors.

^ 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 potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice

@uranusjr

Copy link
Copy Markdown
Member

I feel some of the exceptions shouldn’t be ValueError. This type is for when a value is not right, but some of those aren’t specifically pointing to a value. RuntimeError would be better for at least some cases.

@Taragolis

Copy link
Copy Markdown
Contributor Author

I feel some of the exceptions shouldn’t be ValueError. This type is for when a value is not right, but some of those aren’t specifically pointing to a value. RuntimeError would be better for at least some cases.

Could you point in which cases better use RuntimeError ?

@potiuk

potiuk commented Mar 26, 2024

Copy link
Copy Markdown
Member

Answering for @uranusjr I think possibly yes - those that do not print value that is wrong, could be runtime errors. This is completely incosequential though in dev tools, because those exception are not a 'library" type of exceptions that could be handled by a caller in a different way.

But if I were to made a rule for nit-picking and wanting to be really, really, really strict here:

"if we do not print the value that is wrong and if the value is not directly provided by the user, it should be a runtime error."

But I am ok with any approach here, as it is, as I said inconsquential.

@potiuk

potiuk commented Mar 26, 2024

Copy link
Copy Markdown
Member

BTW. If we want to be REALLY correct here - every time we send a ValueError (i.e. user provided a value that is wrong), we should do:

get_console().print("[error]The value xxxx you provided is wrong, correct value is .....")
sys.exit(1)

Generally speaking raising exception when user input and user console tools is involved, is quite wrong.. So if we want to correct it, then I'd rather say:

  • RuntimeError, whenever it is an irrecoverable error that user has no influence on
  • sys.exit(1) with [error] message and advice to the user in all other cases. will make some suggestions.

Comment thread dev/breeze/src/airflow_breeze/commands/release_management_commands.py Outdated
Comment thread dev/breeze/src/airflow_breeze/commands/release_management_commands.py Outdated
Comment thread dev/breeze/src/airflow_breeze/commands/release_management_commands.py Outdated
Comment thread dev/breeze/src/airflow_breeze/commands/release_management_commands.py Outdated
Comment thread dev/breeze/src/airflow_breeze/global_constants.py Outdated
Comment thread dev/breeze/src/airflow_breeze/utils/docs_publisher.py Outdated
Comment thread dev/breeze/src/airflow_breeze/utils/docs_publisher.py Outdated
Comment thread dev/breeze/src/airflow_breeze/utils/kubernetes_utils.py Outdated
Comment thread dev/breeze/src/airflow_breeze/utils/packages.py Outdated
Comment thread dev/breeze/src/airflow_breeze/utils/packages.py Outdated
Comment thread docs/conf.py Outdated
Comment thread docs/exts/docs_build/docs_builder.py Outdated
Comment thread docs/exts/operators_and_hooks_ref.py Outdated
Comment thread docs/exts/provider_yaml_utils.py Outdated
Comment thread hatch_build.py Outdated
Comment thread scripts/ci/pre_commit/pre_commit_generate_pypi_readme.py Outdated
Comment thread scripts/ci/pre_commit/pre_commit_insert_extras.py Outdated
Comment thread scripts/ci/pre_commit/pre_commit_update_chart_dependencies.py Outdated
Comment thread scripts/ci/pre_commit/pre_commit_update_versions.py Outdated
Comment thread scripts/in_container/update_quarantined_test_status.py Outdated
Comment thread scripts/in_container/update_quarantined_test_status.py Outdated
@potiuk

potiuk commented Mar 26, 2024

Copy link
Copy Markdown
Member

Added my suggestions. @uranusjr -> maybe you can review those and say what you think?

@apache apache deleted a comment from potiuk Mar 26, 2024
Comment thread scripts/ci/pre_commit/pre_commit_json_schema.py Outdated
@potiuk

potiuk commented Apr 2, 2024

Copy link
Copy Markdown
Member

Should we just complete thaat one :) ?

@Taragolis Taragolis force-pushed the too-broad-exceptions-dev branch 2 times, most recently from f1a9faf to 4df4175 Compare April 3, 2024 09:36

@Taragolis Taragolis left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I hope I do not miss any suggested changes

Comment thread docs/exts/docs_build/docs_builder.py Outdated
@Taragolis

Copy link
Copy Markdown
Contributor Author

The error in the tests not related to this changes

Taragolis and others added 3 commits April 4, 2024 21:04
Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@Taragolis Taragolis force-pushed the too-broad-exceptions-dev branch from 26e77a5 to 74939c9 Compare April 4, 2024 17:04
@Taragolis Taragolis merged commit cc901cd into apache:main Apr 4, 2024
@Taragolis Taragolis deleted the too-broad-exceptions-dev branch April 4, 2024 20:27
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jun 3, 2024
ephraimbuddy pushed a commit that referenced this pull request Jun 5, 2024
* Replace to broad exceptions into the Dev

* Add changes according to the review

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

* RuntimeError -> SystemExit if detect it run in __main__

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

---------

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
(cherry picked from commit cc901cd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) kind:documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants