-
-
Notifications
You must be signed in to change notification settings - Fork 210
[ENH] Replace asserts with proper if else Exception handling
#1589
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @fkiraly !! I would like to know you suggestion on this .
In |
asserts with proper if else error handlingasserts with proper if else Exception handling
fkiraly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looks reasonable for me for a first go, to isolate the if/else in a function.
More generally, from an architecture standpoint however, whenever I see a long list of if/elses on the argument class type, I think it is too high coupling and it should be either a method of the (task/argument) class or a visitor pattern.
Why: imagine you want to add a new task. Now you have to add another elif in all of these functions. This is absolutely not extensible.
This should be refactored - I would say, in another PR, after opening an issue with a plan - so the task itself, or a visitor, manages whatever is in the if/else.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1589 +/- ##
==========================================
- Coverage 52.72% 52.64% -0.09%
==========================================
Files 36 36
Lines 4326 4335 +9
==========================================
+ Hits 2281 2282 +1
- Misses 2045 2053 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
asserts with proper if else Exception handlingasserts with proper if else Exception handling

Fixes #1581