OpenBB
OpenBB copied to clipboard
OpenBB-finance
Return None instead of empty list when exception is caught
Description
This PR updates the log_start_end decorator to return None instead of an empty list. This is preferred, because calling functions should be checking for None returns as part of good error checking. However, requiring they check for [] introduces complexity and potential bugs, since an empty list might not be a reasonable thing for a calling function to expect its child function to return.
For example, consider the function below:
@log_start_end(log=logger)
def foo(a,b)
return a / b
In the normal case, this will return a/b as expected. The calling function will check for a float value typically. In the case where b=0 and division by 0 occurs, foo will raise an exception, the decorator will catch it, and [] will be returned. Any function that calls foo is expecting a float to be returned. If None is returned, that indicates an error condition. However, if [] is returned, that is not immediately obvious what went wrong and what is happening.
Here is another example that caused me to make this PR:
@log_start_end(log=logger)
def foo() -> pd.DataFrame
df = pd.DataFrame()
.. do some logic to populate df..
return df
results = foo()
if not foo.empty:
... do something ...
The above will not work. In the case of an exception being raised in foo, then [] will be returned, which will not have a .empty member to look at, causing another exception. A caller will not have logic to check for the list type, since the function is supposedly returning pd.DataFrame as well. It's also not ok to do if foo, since then pandas will throw an error about how DataFrames have multiple ways to check for emptyness (tl;dr, use foo.empty). So instead, the end result will be something like if len(foo) !=0 and not foo.empty, so that the check works for both types that can be returned, i.e. a list and a dataframe. This is error prone and adds complexity.
How has this been tested?
Local testing and pytests.
Others
- [x] I have performed a self-review of my own code.
- [x] I have commented my code, particularly in hard-to-understand areas.
- [x] My code passes all the checks pylint, flake8, black, ... To speed up development you should run
pre-commit install. - [x] New and existing unit tests pass locally with my changes. You can test this locally using
pytest tests/....
This makes sense to me. I agree with your comments.
@Chavithra can we go ahead and merge this? Or is there anything we should be aware from the analytics side?
This makes sense to me. I agree with your comments.
@Chavithra can we go ahead and merge this? Or is there anything we should be aware from the analytics side?
Test this, I had some strange things in Terminal when quit between commands after loading stocks and errors
Error: 'NoneType' object has no attribute 'empty'
2022 Jul 01, 04:26 (🦋) /stocks/ $ q Error: can only join an iterable
2022 Jul 01, 04:26 (🦋) /stocks/ $
It loops back and will not come out of menu.
So I assume [] being an iterable there is an issue with None
My errors are from stock_helper.py Load function which is not decorated
Yup, I think I know what is the issue.
When an user adds an input with "/" we basically split that into commands. And we don't have a global variable but instead process that first command and then retrieve the rest of the queue so that can be processed later on.
I believe that this is why loggers_input returns []
Yup, I think I know what is the issue.
When an user adds an input with "/" we basically split that into commands. And we don't have a global variable but instead process that first command and then retrieve the rest of the queue so that can be processed later on.
I believe that this is why loggers_input returns
[]
Given this how do we resolve returning errors to the display after a try/catch when return?
Error: 'NoneType' object has no attribute 'empty'
Given this how do we resolve returning errors to the display after a try/catch when return?
Error: 'NoneType' object has no attribute 'empty'
IIRC the error checking in models looks if any data was retrieved (sometimes in try-except blocks) and returns an empty dataframe that is checked against .empty in the view
hope this note helps
I've done some tests:
With this being said, I think the problem isn't really if we return None or [] but the way we check if it's, or it's not empty afterwards (as both have the same logical value).
Am I missing something?
Also, I don't see any problem on the analytics side, as we'll be logging the exception anyway.
I think this log_start_end should have re-raise the exception and not returning anything.
What about the consequences of changing it to return None?
For now I think it's better to keep [], until someone takes the time to understand the repercussion of this change or refactor this log_start_end function.
Adding this on possible improvements.
Closing the PR for now.
