[ENH] Replace asserts with proper if else Exception handling#1589
[ENH] Replace asserts with proper if else Exception handling#1589Omswastik-11 wants to merge 10 commits intoopenml:mainfrom
asserts with proper if else Exception handling#1589Conversation
|
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.
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.08% 52.00% -0.09%
==========================================
Files 36 36
Lines 4360 4369 +9
==========================================
+ Hits 2271 2272 +1
- Misses 2089 2097 +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
geetu040
left a comment
There was a problem hiding this comment.
Good, just ran the tests in tests/test_runs/ to be sure of the refactor, and it's green.
Ready to be merged!

Fixes #1581