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

refactor broad exception clauses #276

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HerrMuellerluedenscheid
Copy link
Contributor

@HerrMuellerluedenscheid HerrMuellerluedenscheid commented Nov 7, 2021

Refactored a couple of exception clauses. I also removed some except Exception entirely. They should be used with a lot of care and in cases where there is absolutely no way of getting around this, they should be documented within the code.

@HerrMuellerluedenscheid
Copy link
Contributor Author

Has it already payed off? https://github.com/priv-kweihmann/oelint-adv/runs/4132063894?check_suite_focus=true

Is this an expected exception @priv-kweihmann?

@priv-kweihmann
Copy link
Owner

Also here I have to veto against merging it.
The exception inside of the rule loading method has to be safe so users can run the tool even when a single (custom) is not loadable (there used to be a warning print out for that).

Also the run-method inside of main might be imported to a different utility so this has be safe and not only the outer scope/calling function.

@HerrMuellerluedenscheid
Copy link
Contributor Author

Also here I have to veto against merging it. The exception inside of the rule loading method has to be safe so users can run the tool even when a single (custom) is not loadable (there used to be a warning print out for that).

Also the run-method inside of main might be imported to a different utility so this has be safe and not only the outer scope/calling function.

TBH: I find that pretty speculative and not a reasonable justification for using this construction of try-except which will (and apparently has) lead to hard to trace or even undiscovered errors! But as you like

@HerrMuellerluedenscheid
Copy link
Contributor Author

Re-opening after todays call. Exceptions need to be caught when loading rules to ensure overall stable program executiong but have to be communicated very prominent in the logs/terminal output.

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.

2 participants