-
Notifications
You must be signed in to change notification settings - Fork 20
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
Remove no selected columns warning #325
Conversation
gsheni
commented
Oct 28, 2020
- Closes DataTable.select warns if no columns match #322
Codecov Report
@@ Coverage Diff @@
## main #325 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 2788 2741 -47
=========================================
- Hits 2788 2741 -47
Continue to review full report at Codecov.
|
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.
Maybe we should update the DataTable.select
docstring to mention that if no columns are found matching the specified selectors that an empty DataTable will be returned? Not sure if that is necessary, but might be helpful to users.
@@ -1670,37 +1619,6 @@ def test_select_list_inputs(sample_df): | |||
assert 'signup_date' in dt_common_tags.columns | |||
|
|||
|
|||
def test_select_warnings(sample_df): |
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.
What to you think about leaving this test in place, renaming it to something more appropriate and just removing the with pytest.warns
blocks. That way we have a test that confirms the proper behavior when we use an invalid selector, either alone or in combination with other valid selectors? I'm not sure we are testing that anywhere else.
Also, might consider adding assertions to confirm the underlying dataframe is empty or has the correct columns as well, in addition to just checking the DataTable.columns
.
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.
Added test back, and renamed it. It also handles the situation of checking that empty datatable is returned.
I think we should also remove the warning here. It is basically warning of the same thing, except inside of |
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.
Looks good!